Fix a ton of PEP-8 issues.

Review Request #5509 — Created Feb. 19, 2014 and submitted

Information

Review Board
release-2.0.x
734fbdf...

Reviewers

Since ReviewBot has been slacking off, a bunch of issues have slipped in (plus
there are a lot that we just never fixed up). This change fixes almost
everything, with the exception that a few things PEP-8 thinks are problems are
actually preferred.

Ran unit tests.

Description From Last Updated

Can you add a blank line below this?

chipx86chipx86

Only our unit tests handle the summary broken across lines. We shouldn't do it anywhere else. Same below. Maybe just …

chipx86chipx86

The trailing """ should be on the next line in these cases. The format we're using in the webapi tests …

chipx86chipx86

Maybe just put the """ on the next line? That way it's easily read as one line.

chipx86chipx86

Should shorten the docstring, since this isn't a test, so anything that displays a summary will chop this off. I'm …

chipx86chipx86

I know pep8 likes this, but I despise it. It trades off one readability issue for another, gives us less …

chipx86chipx86

The trailing """ should be on the next line.

chipx86chipx86

""" on next line. Same throughout the file. Can you also move the test conditions to the second line for …

chipx86chipx86

Trailing """

chipx86chipx86

This is less readable.

chipx86chipx86

Must be one line.

chipx86chipx86

One line.

chipx86chipx86

Trailing """. Same below.

chipx86chipx86

One line.

chipx86chipx86

I didn't comment elsewhere about this, but it'd be nice to have the % on the following line, to both …

chipx86chipx86

""" on next line.

chipx86chipx86

""" on next line. Same below.

chipx86chipx86

Must be one line.

chipx86chipx86

One line.

chipx86chipx86

""" on next line. Can put the condition there too.

chipx86chipx86

""" on next line.

chipx86chipx86

Can you put the ] on the following line?

chipx86chipx86

Why is the for on this line? I'd expect it on the next.

chipx86chipx86

Can we remove the indentation change here?

chipx86chipx86
chipx86
  1. Let me just state that while I like code cleanup, I do not like the pep8 checker. Mostly because it's not the PEP-8 checker. It's the "Lots of things I, the author of the pep8 checker, like in my code, plus some things that are actually PEP-8."

    Most of the comments here have to do with docstrings, which need to stay in a format that won't break API doc building tools (which I have some changes for in my tree). I didn't get every instance, but most of the docstring changes need to be fixed up.

  2. Show all issues

    Can you add a blank line below this?

  3. reviewboard/admin/checks.py (Diff revision 1)
     
     
     
    Show all issues

    Only our unit tests handle the summary broken across lines. We shouldn't do it anywhere else.

    Same below. Maybe just reword these.

  4. reviewboard/attachments/tests.py (Diff revision 1)
     
     
    Show all issues

    The trailing """ should be on the next line in these cases.

    The format we're using in the webapi tests when a unit test docstring needs to be split across lines is to do the "Testing <thing>" on one line and the "with ..." on the next, to make it easier to scan the conditions.

  5. reviewboard/changedescs/tests.py (Diff revision 1)
     
     
     
    Show all issues

    Maybe just put the """ on the next line? That way it's easily read as one line.

  6. reviewboard/cmdline/rbsite.py (Diff revision 1)
     
     
     
    Show all issues

    Should shorten the docstring, since this isn't a test, so anything that displays a summary will chop this off.

    I'm sure this is going to come up throughout the change, but I'm not going to comment on all of them.

    I have some changes I've been iterating on for generating API docs (for extension writers) for bits of our codebase. They grab the summary, and these will fail, so I want to make sure we don't do this ever, unless it's a unit tests.

  7. reviewboard/datagrids/columns.py (Diff revision 1)
     
     
     
    Show all issues

    I know pep8 likes this, but I despise it. It trades off one readability issue for another, gives us less space for conditionals, gets confusing with more complex conditionals, and is just plain ugly.

    My vote is still to ignore these.

    If we need to have some visual separation, then let's add more comments about what we're doing in conditionals, or what the result of the conditional was. We could use more comments in the codebase, particularly for students who are new to it.

    1. I did this for one or two of them and then remembered that I don't particularly like it either and disabled the check. I'll revert these.

  8. reviewboard/diffviewer/tests.py (Diff revision 1)
     
     
     
    Show all issues

    The trailing """ should be on the next line.

  9. reviewboard/hostingsvcs/tests.py (Diff revision 1)
     
     
    Show all issues

    """ on next line.

    Same throughout the file.

    Can you also move the test conditions to the second line for these as well? Same reasoning as one of my above comments.

  10. reviewboard/notifications/tests.py (Diff revision 1)
     
     
     
    Show all issues

    Trailing """

  11. reviewboard/reviews/context.py (Diff revision 1)
     
     
     
    Show all issues

    This is less readable.

  12. reviewboard/reviews/context.py (Diff revision 1)
     
     
     
    Show all issues

    Must be one line.

  13. Show all issues

    One line.

  14. reviewboard/reviews/tests.py (Diff revision 1)
     
     
     
    Show all issues

    Trailing """. Same below.

  15. reviewboard/scmtools/errors.py (Diff revision 1)
     
     
     
    Show all issues

    One line.

  16. reviewboard/scmtools/git.py (Diff revision 1)
     
     
     
    Show all issues

    I didn't comment elsewhere about this, but it'd be nice to have the % on the following line, to both give more room for the string, and to associate "HEAD" with being a format argument.

    There's a lot of places where changes were made like this that I think would be nice to update in that way.

  17. reviewboard/scmtools/tests.py (Diff revision 1)
     
     
     
    Show all issues

    """ on next line.

  18. reviewboard/scmtools/tests.py (Diff revision 1)
     
     
     
    Show all issues

    """ on next line. Same below.

  19. Show all issues

    Must be one line.

  20. reviewboard/webapi/tests/base.py (Diff revision 1)
     
     
     
    Show all issues

    One line.

  21. reviewboard/webapi/tests/mixins_comment.py (Diff revision 1)
     
     
     
    Show all issues

    """ on next line. Can put the condition there too.

  22. Show all issues

    """ on next line.

  23. 
      
david
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/tests.py (Diff revision 2)
     
     
     
    Show all issues

    Can you put the ] on the following line?

  3. reviewboard/reviews/ui/text.py (Diff revision 2)
     
     
    Show all issues

    Why is the for on this line? I'd expect it on the next.

  4. reviewboard/test.py (Diff revision 2)
     
     
     
    Show all issues

    Can we remove the indentation change here?

  5. 
      
david
david
chipx86
  1. Ship It!

  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (28ba236).
Loading...