Conversation
+ travis.yml
Tests can still be run against Python3 with: `tox -e py34`
| - Unix-based OS, such as Mac or Linux (Windows support is unclear at this time.) | ||
| - [git](http://git-scm.com/) | ||
| - [Python 2.7](https://www.python.org/download/releases/2.7/) | ||
| or [Python 3.4](https://www.python.org/downloads/release/python-343/) |
There was a problem hiding this comment.
If tox and Travis are using 3.6, want to bump this?
| @@ -1,10 +1,13 @@ | |||
| from abstract import AbstractClient | |||
| from __future__ import absolute_import | |||
| from .abstract import AbstractClient | |||
There was a problem hiding this comment.
Since we're doing this, could we just use the full paths to the imports?
There was a problem hiding this comment.
Probably? This came from the earlier Python 3 PR, so it's not code I wrote. I'll take a look, though
| from abstract import AbstractClient | ||
| from __future__ import print_function | ||
| from __future__ import absolute_import | ||
| from .abstract import AbstractClient |
| @@ -1,3 +1,6 @@ | |||
| from __future__ import print_function | |||
| from __future__ import absolute_import | |||
| from builtins import object | |||
There was a problem hiding this comment.
Does it work that these imports are above the #! and coding lines?
| from .parser import Parser | ||
| from .commit_parser import CommitParser | ||
| from .terms_collector import TermsCollector | ||
| from .clouseau_model import ClouseauModel |
| @@ -1,3 +1,4 @@ | |||
| from builtins import object | |||
There was a problem hiding this comment.
Same as above re: placement above the #!.
| import subprocess | ||
| import pprint | ||
| from clouseau_model import ClouseauModel | ||
| from .clouseau_model import ClouseauModel |
| output = self.get_commit(git_dir, rev) | ||
| if output.strip() == '': | ||
| print "WARNING: No output was returned from git for commit [%s]. Ensure the commit exists" % rev | ||
| print("WARNING: No output was returned from git for commit [%s]. Ensure the commit exists" % rev) |
There was a problem hiding this comment.
Could we use .format() string formatting? From there it's not a huge jump to f-strings when we go python 3.6+ only 😈 .
| (out,err) = git_show.communicate() | ||
| if err: | ||
| print "ERROR running git command [%s]: %s" % (git_show_cmd, err) | ||
| print("ERROR running git command [%s]: %s" % (git_show_cmd, err)) |
There was a problem hiding this comment.
Same as above re: string formatting.
| line = str( line, 'utf-8' ) | ||
| except UnicodeDecodeError: | ||
| line = unicode( line, 'latin-1' ) | ||
| line = str( line, 'latin-1' ) |
There was a problem hiding this comment.
More a comment than anything specific to this PR, this seems like an incredibly fragile way to test encoding.
3 participants
This incorporates work that @sking963 submitted in #35 (thank you! belatedly!), some tox additions I made in #34, and some tweaks to simplify subprocess handling (Python 3.6 was complaining about how the existing code attempted to join bytes and a string).