-
-
-
Only our unit tests handle the summary broken across lines. We shouldn't do it anywhere else.
Same below. Maybe just reword these.
-
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.
-
-
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.
-
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.
-
-
"""
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.
-
-
-
-
-
-
-
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.
-
-
-
-
-
-
Fix a ton of PEP-8 issues.
Review Request #5509 — Created Feb. 19, 2014 and submitted
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? |
chipx86 | |
Only our unit tests handle the summary broken across lines. We shouldn't do it anywhere else. Same below. Maybe just … |
chipx86 | |
The trailing """ should be on the next line in these cases. The format we're using in the webapi tests … |
chipx86 | |
Maybe just put the """ on the next line? That way it's easily read as one line. |
chipx86 | |
Should shorten the docstring, since this isn't a test, so anything that displays a summary will chop this off. I'm … |
chipx86 | |
I know pep8 likes this, but I despise it. It trades off one readability issue for another, gives us less … |
chipx86 | |
The trailing """ should be on the next line. |
chipx86 | |
""" on next line. Same throughout the file. Can you also move the test conditions to the second line for … |
chipx86 | |
Trailing """ |
chipx86 | |
This is less readable. |
chipx86 | |
Must be one line. |
chipx86 | |
One line. |
chipx86 | |
Trailing """. Same below. |
chipx86 | |
One line. |
chipx86 | |
I didn't comment elsewhere about this, but it'd be nice to have the % on the following line, to both … |
chipx86 | |
""" on next line. |
chipx86 | |
""" on next line. Same below. |
chipx86 | |
Must be one line. |
chipx86 | |
One line. |
chipx86 | |
""" on next line. Can put the condition there too. |
chipx86 | |
""" on next line. |
chipx86 | |
Can you put the ] on the following line? |
chipx86 | |
Why is the for on this line? I'd expect it on the next. |
chipx86 | |
Can we remove the indentation change here? |
chipx86 |
- Change Summary:
-
Make all requested changes.
- Branch:
-
masterrelease-2.0.x
- Commit:
-
5674f2d5d13fc0ceb7d12350e7d195320ab63efa734fbdf5d74c0b518aeafb1de1f915866e96c6ac
- Diff:
-
Revision 2 (+497 -434)