Skip to content

Large PR -- Adding Tests, fixing a few bugs that revealed, and cleaning up small nits #2

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

Merged
merged 7 commits into from
May 12, 2016

Conversation

t8y8
Copy link
Contributor

@t8y8 t8y8 commented May 10, 2016

Sorry, this PR grew, I'm trying to remember my git-fu.

I'll start putting changes into their own branches and PRs :)

Changes:

  • PEP8 Autoformat from Sublime (cleaned up some whitespace and line lengths)
  • Make _is_valid_file a staticmethod since it doesn't need access to the instance
  • Remove the C++-ism from _is_valid_file
  • Updated the Replicate Workbook Example to be a bit cleaner using DictReader and removing a copy
  • Small refactor to save_as
  • Added way more tests so that at least some basics are covered with a sample twb and tds. These even uncovered a bug in Datasource that I fixed

Tests pass before and after change on python2 and python3

t8y8 added 2 commits May 10, 2016 16:17
…file and add some test cases for a help method I plan to update in the next commit. I made _is_valid_file a static method since it's just a helper and it makes it easier to test
…more filetypes in the future. No more C++ code in the python :)
t8y8 added 3 commits May 10, 2016 17:51
… code more readable. Also remove the unnecessary copy.copy call and import. Small refactor of parameter name for Workbook.save_as method to be more descriptive than 'value'. Manual testing on python2 and pyhon3 for example. Verified still works
…tion classes. The temp-file setUp isn't the greatest but it's functional for now and will help with confidance if we decide to move anything around. Already uncovered a bug with them, fixed in this commit -- TDS files don't seem to have a 'name' attribute but instead call it 'formatted-name' I added a simple OR into the _name logic of the Datasource class and that resolves the bug.
@t8y8 t8y8 changed the title Refactor _is_valid_file and add tests Large PR -- Adding Tests, fixing a few bugs that revealed, and cleaning up small nits May 11, 2016
class DatasourceModelTests(unittest.TestCase):

def setUp(self):
self.tds_file = tempfile.NamedTemporaryFile(suffix='.tds')
Copy link
Contributor Author

@t8y8 t8y8 May 11, 2016

Choose a reason for hiding this comment

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

My tests didn't work on Windows due to the way Windows handles the namedtemporaryfile.

I manually manage the files now and it works cross platform

…managing temp files right now, but we may want to modify our Workbook and Datasource classes to accept all File-Like objects in the future so I don't have to do this filename sillyness. Still, this works for now and is cross platform
@benlower benlower merged commit d6c27d8 into tableau:master May 12, 2016
@benlower
Copy link
Contributor

merging this. let's talk about the 'file-like' objects comment soon

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

Successfully merging this pull request may close these issues.

3 participants