Fix various PEP 8, style and PyFlakes issues

Review Request #9655 — Created Feb. 16, 2018 and submitted

Information

RBTools
master
6540441...

Reviewers

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 0

Note: 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,
ok

Ran 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 …

chipx86chipx86

We have a certain way we like to see content for review request summaries, descriptions, and Testing Done. The descriptions …

chipx86chipx86

You really don't need the entire output of the tests in the testing done. You can just say you ran …

brenniebrennie

Can you revert this? In cases like this, we prefer the variable on its own line, to give room for …

chipx86chipx86

Same here.

chipx86chipx86

We ignore the pep8 tool here. These should always be aligned, no indentation.

chipx86chipx86

No indentation here, either.

chipx86chipx86

Nor here.

chipx86chipx86

Nor here.

chipx86chipx86

We actually shouldn't have a period at the end of this, since it's for a unit test. Remove that and …

chipx86chipx86

No indentation.

chipx86chipx86

Nor here.

chipx86chipx86

Nor these. Mind going through the rest of the change and undoing these? It will help. I don't want to …

chipx86chipx86

This can be changed to: if sys.platform.startswith(('win', 'cygwin')):

chipx86chipx86

In cases like this, we prefer the % on the following line, like: raise SCMError('...' % var)

chipx86chipx86

This should be undone. Logging arguments on the line after the string.

chipx86chipx86

The old way was preferable.

chipx86chipx86

Here, too.

chipx86chipx86

We prefer the old format.

chipx86chipx86

Shouldn't be indented.

chipx86chipx86

Can you rever this? We spread out our dictionaries, so that further changes don't affect as much code.

chipx86chipx86

Each line should have the r prefix.

chipx86chipx86

I'm not sure this is no longer the case, for Python 2 support. We needed this to fix a bug. …

chipx86chipx86

While split across more lines, the previous version was fine. Easier to scan.

chipx86chipx86

Please revert these. Long lines are fine if they're a URL, since we want them to be proper links. In …

chipx86chipx86

Individual test case docstrings should not end in a period (the ones that are "Testing XXXX"), because the test runner …

daviddavid

Add a blank line above this one.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Add a blank line above this one.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Period here.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

This is copy/pasted output from the command. I don't think we should change it.

daviddavid

This is copy/pasted output from the command. I don't think we should change it.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

Revert this change.

daviddavid

This is a wildcard and so should be under the list of wildcards.

brenniebrennie

Instead, how about: content.write(b'%s%s' % (six.text_type(self._fields[key]).encode('utf-8'), NEWLINE)

brenniebrennie

Instead, how about: content.write(b'filename="%s"%s' % (filename.encode('utf-8'), NEWLINE))

brenniebrennie

The way this is wrapped is now more confusing. How about: content.write( six.text_type(self._fields[key]).encode('utf-8') + NEWLINE)

daviddavid

To be consistent with our other configurations, let's do E121,E125,E129,E241

daviddavid

This still isn't how I suggested you wrap it. content.write( six.text_type(self._fields[key]).encode('utf-8') + NEWLINE)

daviddavid

This can be a little bit more compact: if (git_top.startswith(('fatal:', 'cygdrive')) or not os.path.isdir(git_dir)):

daviddavid

Extra line here isn't necessary unless there's a class docstring.

daviddavid
solarmist
chipx86
  1. Hi,

    Thanks for the contributions! David's been working on Python 3 support, with changes coming, so I'm not sure how much of these might collide, but it looks like there's a lot of great cleanup and improvements for some of the very old code in this codebase that I'd be glad to incorporate.

    I want to get a sense of whether we should expect further changes as part of this series, or if this is the complete set, to avoid redundant work. We might want to coordinate more over e-mail, just in case.

    There's a lot in this particular change that fix complaints from the pep8/pycodestyle warnings, but they are not things we choose to follow in our codebase. For example, you'll see a handful of comments about changes like:

    if (a and
            b):
    

    We don't follow this style, and Review Bot doesn't warn about it on our server. We prefer to keep things aligned. There's a handful of other changes like that in this review request that we'll want to see some tweaks to, and some we'll have you outright remove. If you wouldn't mind making similar changes to the other review requests, that will definitely help us review and incorporate these into the codebase. Hope you don't mind, and know we appreciate the work that went into this :)

    1. This should be the only "bulk" change and everything moving forward would be fixing individual bugs or adding test coverage where it is lacking.

  2. Show all issues

    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?

    1. Sure, I was a bit unsure about it since this was one very large change broken down into pieces.

      Updated for this review. I'm dropping this issue and will update each review accordingly as I fix issues.

  3. rbtools/api/cache.py (Diff revision 1)
     
     
     
    Show all issues

    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.

  4. rbtools/api/cache.py (Diff revision 1)
     
     
     
    Show all issues

    Same here.

  5. rbtools/api/request.py (Diff revision 1)
     
     
     
    Show all issues

    We ignore the pep8 tool here. These should always be aligned, no indentation.

  6. rbtools/api/resource.py (Diff revision 1)
     
     
     
    Show all issues

    No indentation here, either.

  7. rbtools/api/resource.py (Diff revision 1)
     
     
     
     
    Show all issues

    Nor here.

  8. rbtools/api/resource.py (Diff revision 1)
     
     
     
    Show all issues

    Nor here.

  9. rbtools/api/tests.py (Diff revision 1)
     
     
     
    Show all issues

    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.

  10. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
    Show all issues

    No indentation.

  11. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
    Show all issues

    Nor here.

  12. rbtools/clients/__init__.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues

    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 :)

  13. rbtools/clients/clearcase.py (Diff revision 1)
     
     
     
    Show all issues

    This can be changed to:

    if sys.platform.startswith(('win', 'cygwin')):
    
    1. That's handy. I wasn't aware of that.

  14. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
    Show all issues

    In cases like this, we prefer the % on the following line, like:

    raise SCMError('...'
                   % var)
    
  15. rbtools/clients/perforce.py (Diff revision 1)
     
     
     
     
    Show all issues

    This should be undone. Logging arguments on the line after the string.

  16. rbtools/clients/plastic.py (Diff revision 1)
     
     
     
    Show all issues

    The old way was preferable.

  17. rbtools/clients/plastic.py (Diff revision 1)
     
     
     
    Show all issues

    Here, too.

  18. rbtools/clients/svn.py (Diff revision 1)
     
     
     
    Show all issues

    We prefer the old format.

  19. rbtools/clients/svn.py (Diff revision 1)
     
     
     
    Show all issues

    Shouldn't be indented.

  20. rbtools/clients/tests/test_svn.py (Diff revision 1)
     
     
     
     
    Show all issues

    Can you rever this? We spread out our dictionaries, so that further changes don't affect as much code.

  21. rbtools/clients/tfs.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Each line should have the r prefix.

  22. rbtools/commands/__init__.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    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.

    1. But can you explain more? I can't find any reference to this and I haven't seen any warnings or errors due to this.

    2. On Python 2.x, we were hitting a problem where byte strings containing non-ASCII characters were being implicitly converted to Unicode strings due to being concatenated onto RB_MAIN (which was a Unicode string and was taking precedence due to being first).

      The reason for str() has to do with the differences in default string types on Python 2 and 3. For Python 2, the default type is a byte string (and you have to opt-in to Unicode strings with either a u'...' or by doing from __future__ import unicode_literals. On Python 3, the default is a Unicode string, and you have to opt into a byte string with b'...'.

      str(), in both versions, represents the default type string for the language (not the file). So for Python 2, it gives you byte strings. For Python 3, Unicode strings.

      However, I'm not certain that Unicode strings on Python 3 are safe here. It might still trigger the same bug, because we're dealing with data from an unknown locale.

      For now, I'd like to revert this change and instead see it fixed along with new unit tests that successfully initially reproduce the original problem on Python 2 and see if it (and a fix) impacts Python 3.

    3. Removed from this commit. I wasn't running into this at all in my testing. It probably has to do with the terminal LANG.

    4. Yeah, I know the original repro case was from a Chinese user who was setting a default summary or description through the command line arguments (--summary, --description) to Chinese text, and I don't believe the encoding was UTF-8.

  23. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues

    While split across more lines, the previous version was fine. Easier to scan.

  24. rbtools/utils/appdirs.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    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.

    1. Done. Dropped for all style changes.

  25. 
      
chipx86
  1. 
      
  2. Show all issues

    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.

    1. I assume this is referring to the other changes as this commit is purely a style change and flake8 would pickup any syntax errors.

    2. This mostly refers to the other changes, but I just left a note about how the RB_MAIN change impacts things and the testing needed. Anything that is not purely style (anything changing string types, for instance) requires investigation into the need for a certain type and then unit tests around it.

    3. I will at a minimimum move this out of this change.

  3. 
      
solarmist
solarmist
david
  1. 
      
  2. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    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.

    1. I'm fine with making these changes and while most people won't be attempting to make large scale changes to code like I am, I feel like this kind of rule adds unneeded sublty to docstring in unit test files.

      I'd propose for simplicity that in test files the summary lines should exclude the period. Otherwise the documentation should be updated to call out this exception for test cases.

  3. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Add a blank line above this one.

  4. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  5. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  6. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  7. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  8. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  9. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Add a blank line above this one.

  10. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  11. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  12. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  13. rbtools/api/tests.py (Diff revision 2)
     
     
    Show all issues

    Period here.

  14. rbtools/clients/tests/__init__.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  15. rbtools/clients/tests/__init__.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  16. rbtools/clients/tests/test_bzr.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  17. rbtools/clients/tests/test_bzr.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  18. rbtools/clients/tests/test_bzr.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  19. rbtools/clients/tests/test_bzr.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  20. rbtools/clients/tests/test_git.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  21. rbtools/clients/tests/test_git.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  22. rbtools/clients/tests/test_git.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
    Show all issues

    This is copy/pasted output from the command. I don't think we should change it.

    1. I don't think materially changes the output, unless you expect someone to try an exact character match of this text.

      I feel like it should be alright to make this kind of whitespace change.

  23. rbtools/clients/tests/test_git.py (Diff revision 2)
     
     
     
    Show all issues

    This is copy/pasted output from the command. I don't think we should change it.

    1. Again I feel like this change is immaterial to it's contents and except for a whitespace difference git itself swaps between this format and the one in the previous comment.

  24. rbtools/clients/tests/test_mercurial.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  25. rbtools/clients/tests/test_mercurial.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  26. rbtools/clients/tests/test_mercurial.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  27. rbtools/clients/tests/test_p4.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  28. rbtools/clients/tests/test_p4.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  29. rbtools/clients/tests/test_p4.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  30. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  31. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  32. rbtools/clients/tests/test_svn.py (Diff revision 2)
     
     
    Show all issues

    Revert this change.

  33. 
      
solarmist
solarmist
solarmist
solarmist
brennie
  1. 
      
  2. Show all issues

    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.

    1. Done. I just left tests that had any output. Most of it is just python 3 warnings.

  3. .gitignore (Diff revision 5)
     
     
    Show all issues

    This is a wildcard and so should be under the list of wildcards.

  4. rbtools/api/request.py (Diff revision 5)
     
     
     
    Show all issues

    Instead, how about:

    content.write(b'%s%s'
                  % (six.text_type(self._fields[key]).encode('utf-8'),
                  NEWLINE)
    
    1. I have a seperate review to refactor this code, so I'd like to only do the formatting update in this change.

  5. rbtools/api/request.py (Diff revision 5)
     
     
     
    Show all issues

    Instead, how about:

    content.write(b'filename="%s"%s'
                  % (filename.encode('utf-8'), NEWLINE))
    
  6. 
      
solarmist
solarmist
solarmist
david
  1. 
      
  2. rbtools/api/request.py (Diff revision 7)
     
     
     
    Show all issues

    The way this is wrapped is now more confusing. How about:

    content.write(
        six.text_type(self._fields[key]).encode('utf-8') +
        NEWLINE)
    
  3. setup.cfg (Diff revision 7)
     
     
    Show all issues

    To be consistent with our other configurations, let's do E121,E125,E129,E241

  4. 
      
solarmist
david
  1. 
      
  2. rbtools/api/request.py (Diff revision 8)
     
     
     
     
    Show all issues

    This still isn't how I suggested you wrap it.

    content.write(
        six.text_type(self._fields[key]).encode('utf-8') +
        NEWLINE)
    
    1. This is also not a useful comment, let alone a big enough to warrent an issue. It is a completely non-functional change that doesn't significantly alter the readibility of the statement. The best I can say is that it's different.

  3. rbtools/clients/git.py (Diff revision 8)
     
     
     
     
    Show all issues

    This can be a little bit more compact:

    if (git_top.startswith(('fatal:', 'cygdrive')) or
        not os.path.isdir(git_dir)):
    
    1. I always forget about this version of the function. It's a bit strange to need to wrap it in a tuple.

  4. rbtools/clients/tests/test_svn.py (Diff revision 8)
     
     
    Show all issues

    Extra line here isn't necessary unless there's a class docstring.

  5. 
      
solarmist
david
  1. Ship It!
  2. 
      
solarmist
Review request changed
Status:
Completed
Change Summary:
Pushed to master (0c562dc)