Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commenting and Docstring cleanup. A few very small code cleanups #120

Merged
merged 2 commits into from
Dec 14, 2016
Merged

Commenting and Docstring cleanup. A few very small code cleanups #120

merged 2 commits into from
Dec 14, 2016

Conversation

t8y8
Copy link
Contributor

@t8y8 t8y8 commented Dec 12, 2016

In the interest of making the library as approachable as possible for users and contributors, I took a pass at adding in more docstrings and comments where I felt appropriate.

Finding the right voice is hard, so I'm not married to my particular text.

Fields is already pretty well documented, but @RussTheAerialist may have updates in mind.

Ok, cheated and changed a few very minor pieces of code as well, I made a function 'private' since it really should be, and got rid of some extra parens.

@@ -87,27 +87,32 @@ def base36encode(number):
return sign + base36


def make_unique_name(dbclass):
def _make_unique_name(dbclass):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't ever be called outside of the class so I made it private.


dsxml = xml_open(filename, cls.__name__.lower()).getroot()
dsxml = xml_open(filename, 'datasource').getroot()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that this was actually less readable than just making it a literal string, which removed the need for the comment "#Gets the name of the class and normalizes it so it can verify the file it opens"

@@ -234,16 +235,15 @@ def clear_repository_location(self):
if tag is not None:
self._datasourceXML.remove(tag)

###########
# fields
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these added more clutter than they helped, and weren't consistently applied.

I left the section headers in place (Public API, Private API)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then I realized I left several in. I'll remove them

@property
def dbclass(self):
"""The type of connection (e.g. 'MySQL', 'Postgresql')."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a list of all available options somewhere? if so would be nice to point ppl to that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dbclass.py file lists my best guess at a comprehensive list that I extracted from our CPP code :), I can point them to that

A class for writing Tableau workbook files.

"""
"""A class for writing Tableau workbook files."""

###########################################################################
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so are we leaving this comment in to denote the public API? fine if we do just checking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the public and private api bits in, I personally am not a fan, so I'm happy to remove them.

But I was deferring to your original commit 🍭


# Is the file a zip (twbx or tdsx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's be consistent with what is in datasource.py and prepend each extension with a ".". so make this ".twbx or .tdsx" please

Copy link
Contributor

@benlower benlower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 with a couple of nits

Copy link
Contributor

@jdomingu jdomingu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🚀

@t8y8 t8y8 merged commit fe16c0c into tableau:development Dec 14, 2016
@t8y8 t8y8 deleted the feature-docstrings branch December 14, 2016 00:38
graysonarts pushed a commit that referenced this pull request Jan 11, 2017
* Fix #117 by only attempting files with the right extension inside the archive (#118)

* Commenting and Docstring cleanup. A few very small code cleanups (#120)

Add docstrings and remove clutter. I also made some very tiny tweaks to some code for clarity.

* Small cleanups for various editors. Play nice with built in test-runners (#121)

* Add Py36, update travis to use pycodestyle (#124)

* Add `initial sql` and `query band` support (#123)

Addresses #109 and #110

* Prep for release of 0.6 (#125)

* Prep for release of 0.6

* wordsmithing the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants