Fix various PEP 8, style and PyFlakes issues
Review Request #9655 — Created Feb. 16, 2018 and submitted
Information | |
---|---|
solarmist | |
RBTools | |
master | |
|
|
9648 | |
6540441... | |
Reviewers | |
rbtools | |
Fix various PEP 8, sytle and PyFlakes issues.
I've also included an exception for E129 in the setup.cfg to make it explicit in the codebase
that this is the expected style.
(%) flake8 -v tests rbtools
flake8.checker MainProcess 409 INFO Making checkers
flake8.checker MainProcess 650 INFO Checking 79 files
flake8.main.application MainProcess 2483 INFO Finished running
flake8.main.application MainProcess 2484 INFO Reporting errors
flake8.main.application MainProcess 2485 INFO Found a total of 37 violations and reported 0Note: These violations were in appdirs.py and the docs directory.
time ./tests/runtests.py
/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/pyversion.py:36: DeprecationWarning: The 'new' module has been removed in Python 3.0; use the 'types' module instead.
import new
/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/plugins/prof.py:14: DeprecationWarning: The 'hotshot' module is not supported in 3.x, use the 'profile' module instead.
import hotshot
/Volumes/Augie/solarmist/rbtools/lib/python2.7/site-packages/nose/pyversion.py:49: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
class Key(object):
#1 Testing the cache with a high max-age value ... /Volumes/Augie/solarmist/rbtools/rbtools/api/cache.py:479: DeprecationWarning: buffer() not supported in 3.x
sqlite3.Binary(entry.response_body)))
ok
#2 Testing the cache with a zero max-age value ... /Volumes/Augie/solarmist/rbtools/rbtools/api/cache.py:493: DeprecationWarning: buffer() not supported in 3.x
sqlite3.Binary(entry.response_body), entry.url,
okRan 210 tests in 215.007s
OK
./tests/runtests.py 93.50s user 60.71s system 71% cpu 3:35.47 total
Description | From | Last Updated |
---|---|---|
Something else to mention is that, as proof that these changes don't regress Python 2.7 and that they work on … |
|
|
We have a certain way we like to see content for review request summaries, descriptions, and Testing Done. The descriptions … |
|
|
You really don't need the entire output of the tests in the testing done. You can just say you ran … |
|
|
Can you revert this? In cases like this, we prefer the variable on its own line, to give room for … |
|
|
Same here. |
|
|
We ignore the pep8 tool here. These should always be aligned, no indentation. |
|
|
No indentation here, either. |
|
|
Nor here. |
|
|
Nor here. |
|
|
We actually shouldn't have a period at the end of this, since it's for a unit test. Remove that and … |
|
|
No indentation. |
|
|
Nor here. |
|
|
Nor these. Mind going through the rest of the change and undoing these? It will help. I don't want to … |
|
|
This can be changed to: if sys.platform.startswith(('win', 'cygwin')): |
|
|
In cases like this, we prefer the % on the following line, like: raise SCMError('...' % var) |
|
|
This should be undone. Logging arguments on the line after the string. |
|
|
The old way was preferable. |
|
|
Here, too. |
|
|
We prefer the old format. |
|
|
Shouldn't be indented. |
|
|
Can you rever this? We spread out our dictionaries, so that further changes don't affect as much code. |
|
|
Each line should have the r prefix. |
|
|
I'm not sure this is no longer the case, for Python 2 support. We needed this to fix a bug. … |
|
|
While split across more lines, the previous version was fine. Easier to scan. |
|
|
Please revert these. Long lines are fine if they're a URL, since we want them to be proper links. In … |
|
|
Individual test case docstrings should not end in a period (the ones that are "Testing XXXX"), because the test runner … |
|
|
Add a blank line above this one. |
|
|
Period here. |
|
|
Period here. |
|
|
Period here. |
|
|
Period here. |
|
|
Period here. |
|
|
Add a blank line above this one. |
|
|
Period here. |
|
|
Period here. |
|
|
Period here. |
|
|
Period here. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
This is copy/pasted output from the command. I don't think we should change it. |
|
|
This is copy/pasted output from the command. I don't think we should change it. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
Revert this change. |
|
|
This is a wildcard and so should be under the list of wildcards. |
|
|
Instead, how about: content.write(b'%s%s' % (six.text_type(self._fields[key]).encode('utf-8'), NEWLINE) |
|
|
Instead, how about: content.write(b'filename="%s"%s' % (filename.encode('utf-8'), NEWLINE)) |
|
|
The way this is wrapped is now more confusing. How about: content.write( six.text_type(self._fields[key]).encode('utf-8') + NEWLINE) |
|
|
To be consistent with our other configurations, let's do E121,E125,E129,E241 |
|
|
This still isn't how I suggested you wrap it. content.write( six.text_type(self._fields[key]).encode('utf-8') + NEWLINE) |
|
|
This can be a little bit more compact: if (git_top.startswith(('fatal:', 'cygdrive')) or not os.path.isdir(git_dir)): |
|
|
Extra line here isn't necessary unless there's a class docstring. |
|
-
-
We have a certain way we like to see content for review request summaries, descriptions, and Testing Done. The descriptions don't need to reference other review requests (the Depends On field is enough for this), and instead should focus solely on what this change is about. We typically use a format describing, at a medium-to-high level, the problem being addressed by a change and what was done to fix it.
A guide and examples are here: https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea
Mind updating the review requests to fit that format?
-
rbtools/api/cache.py (Diff revision 1) Can you revert this? In cases like this, we prefer the variable on its own line, to give room for expanding the string as necessary. Basically, when there's multiple lines of arguments, we tend to put each on its own line.
-
-
rbtools/api/request.py (Diff revision 1) We ignore the pep8 tool here. These should always be aligned, no indentation.
-
-
-
-
rbtools/api/tests.py (Diff revision 1) We actually shouldn't have a period at the end of this, since it's for a unit test. Remove that and the
"""
will fit on the previous line. -
-
-
rbtools/clients/__init__.py (Diff revision 1) Nor these.
Mind going through the rest of the change and undoing these? It will help. I don't want to flood you with comments :)
-
rbtools/clients/clearcase.py (Diff revision 1) This can be changed to:
if sys.platform.startswith(('win', 'cygwin')):
-
rbtools/clients/perforce.py (Diff revision 1) In cases like this, we prefer the
%
on the following line, like:raise SCMError('...' % var)
-
rbtools/clients/perforce.py (Diff revision 1) This should be undone. Logging arguments on the line after the string.
-
-
-
-
-
rbtools/clients/tests/test_svn.py (Diff revision 1) Can you rever this? We spread out our dictionaries, so that further changes don't affect as much code.
-
-
rbtools/commands/__init__.py (Diff revision 1) I'm not sure this is no longer the case, for Python 2 support. We needed this to fix a bug.
If this is for Python 3 support, a better option would be to do:
RB_MAIN = str('rbt')
This ensures a byte string on Python 2, Unicode string on Python 3. Given that, the comment should remain, but be expanded.
-
rbtools/commands/post.py (Diff revision 1) While split across more lines, the previous version was fine. Easier to scan.
-
rbtools/utils/appdirs.py (Diff revision 1) Please revert these. Long lines are fine if they're a URL, since we want them to be proper links.
In fact, all changes to this file should be reverted. It tracks an upstream package, and we don't want to make changes to it, as it's harder to figure out the differences between versions this way.
-
-
Something else to mention is that, as proof that these changes don't regress Python 2.7 and that they work on Python 3.4, 3.5, and 3.6 (our targets), we'd like to see new unit tests for anything not currently covered. If you have questions on this, please reach out to us, as we know the test coverage in RBTools is not currently as comprehensive as most of our software.
Description: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+93 -89) |
Checks run (2 succeeded)
-
-
rbtools/api/tests.py (Diff revision 2) Individual test case docstrings should not end in a period (the ones that are "Testing XXXX"), because the test runner will add an ellipsis. Other docstrings, like these. should have a period at the end.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
rbtools/clients/tests/test_git.py (Diff revision 2) This is copy/pasted output from the command. I don't think we should change it.
-
rbtools/clients/tests/test_git.py (Diff revision 2) This is copy/pasted output from the command. I don't think we should change it.
-
-
-
-
-
-
-
-
-
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 3 (+67 -60) |
Checks run (2 succeeded)
Change Summary:
Found a few additional spots missing blank lines.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+75 -60) |
Checks run (2 succeeded)
Change Summary:
After updating the setup.cfg I was able to catch the issues in contrib
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Depends On: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+92 -74) |
Checks run (2 succeeded)
Testing Done: |
|
---|
-
-
You really don't need the entire output of the tests in the testing done. You can just say you ran the tests and they passed.
-
-
rbtools/api/request.py (Diff revision 5) Instead, how about:
content.write(b'%s%s' % (six.text_type(self._fields[key]).encode('utf-8'), NEWLINE)
-
rbtools/api/request.py (Diff revision 5) Instead, how about:
content.write(b'filename="%s"%s' % (filename.encode('utf-8'), NEWLINE))
Testing Done: |
|
---|
Change Summary:
Sorted .gitignore file
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+98 -80) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+98 -80) |
Checks run (2 succeeded)
-
-
rbtools/api/request.py (Diff revision 7) The way this is wrapped is now more confusing. How about:
content.write( six.text_type(self._fields[key]).encode('utf-8') + NEWLINE)
-
setup.cfg (Diff revision 7) To be consistent with our other configurations, let's do E121,E125,E129,E241
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+99 -80) |
Checks run (2 succeeded)
-
-
rbtools/api/request.py (Diff revision 8) This still isn't how I suggested you wrap it.
content.write( six.text_type(self._fields[key]).encode('utf-8') + NEWLINE)
-
rbtools/clients/git.py (Diff revision 8) This can be a little bit more compact:
if (git_top.startswith(('fatal:', 'cygdrive')) or not os.path.isdir(git_dir)):
-
rbtools/clients/tests/test_svn.py (Diff revision 8) Extra line here isn't necessary unless there's a class docstring.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+97 -80) |