flake8
-
rbtools/commands/__init__.py (Diff revision 1) Show all issues -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Review Request #11401 — Created Jan. 23, 2021 and submitted
Information | |
---|---|
ryankang | |
RBTools | |
master | |
11521 | |
Reviewers | |
rbtools, students | |
RBTools commands were previously using
sys.stderr
to output messages. This change abstracting outputting by using a
wrapper around a stream output object. This makes it easier to customize
output streams and suppress output. 4 wrappers (2 for standard output
unicode and byte, 2 for standard error unicode and byte) are initiated
in the Command class that is accessible to all child classes.
Ran all tests in
./tests/runtests.py rbtools.commands
and passed.Added two new tests for
__init__.py
. One that tests if output stream
object is set correctly forOutputWrapper
and another test that makes
sureOutputWrapper
passes the correct message to the output stream object
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
All the changes suggested have to do with consistency with self.stdout.new_line(). Other than that, everything else looks good to me! |
|
|
Your summary should be much shorter, about 70 columns. Further detail (for example, what tests were added) should be in … |
|
|
E501 line too long (85 > 79 characters) |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (85 > 79 characters) |
![]() |
|
E501 line too long (88 > 79 characters) |
![]() |
|
E501 line too long (89 > 79 characters) |
![]() |
|
E211 whitespace before '(' |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E501 line too long (89 > 79 characters) |
![]() |
|
E501 line too long (88 > 79 characters) |
![]() |
|
E501 line too long (85 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
E501 line too long (88 > 79 characters) |
![]() |
|
E501 line too long (89 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
W292 no newline at end of file |
![]() |
|
W291 trailing whitespace |
![]() |
|
E124 closing bracket does not match visual indentation |
![]() |
|
E231 missing whitespace after ',' |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
For consistency with the other changes, should this maybe instead be self.stdout.new_line() ? |
|
|
Again for consistency, maybe this should be self.stdout.new_line() ? |
|
|
This should be self.stdout.new_line()? |
|
|
This should be self.stdout.new_line() ? |
|
|
This should also be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
This should be self.stdout.new_line() |
|
|
W292 no newline at end of file |
![]() |
|
W391 blank line at end of file |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (89 > 79 characters) |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
E501 line too long (89 > 79 characters) |
![]() |
|
All test cases should be in files called test_X, not the __init__.py. In this case I think you could probably … |
|
|
A few changes in here: The file needs a module docstring The first thing after that docstring should be from … |
|
|
Dedent this three spaces. |
|
|
Typo: strea -> stream |
|
|
This needs a method docstring. |
|
|
This needs a method docstring. |
|
|
This needs a method docstring. |
|
|
Can we call these _bytes instead of _byte? |
|
|
Let's add another newline in here, before the format operation. Otherwise it's sometimes easy to miss when scanning quickly through … |
|
|
The second line here needs to be indented a bit more: % (review_request.submitter.fullname or review_request.submitter.username)) |
|
|
When we wrap a string, we put the space at the end of the first line. Let's also put the … |
|
|
Please undo the changes here (so the space is at the end of the first line). |
|
|
One thing Christian just pointed out to me is that he's been moving to just import kgb, and then using … |
|
|
Please make sure this is sorted alphabetically. |
|
|
We still need two blank lines here. |
|
|
This isn't a good description. Let's say "Unit tests for command OutputWrapper" |
|
|
Docstrings need to be in the following form: """Single-line summary. Multi-line description. """ Along with this, for any newly-introduced classes … |
|
|
Function docstrings must follow the same form, and specify any arguments, return values, and exceptions raised. Take a look at … |
|
|
We expect a blank line between the end of a block and a new statememt, to help separate that out … |
|
|
There's some typos in these docstrings ("charater" here, "specifiy" in the class docstring). If your editor supports it, I recommend … |
|
|
This docstring is old, and needs to be modernized. As a step toward this, since you're introducing some really important … |
|
|
There's an inconsistency in variable names. You have stderr_bytes (plural) and stderr_byte (singular). The plural form is what we want, … |
|
|
Here's an area where the new output wrappers solve a problem! Not all diffs can be decoded as UTF-8, so … |
|
|
The string is a parameter to textwrap.fill(), rather than a second parameter to stdout.write. Since we're wrapping to the following … |
|
|
To keep lint checkers from complaining about indentations being multiples of 4, this should actually be in the same form … |
|
|
This should be a single import statement. |
|
|
This is a class docstring, so it needs to have a trailing period. |
|
|
All unit test docstrings should be in the form of Testing <Thing> <condition>. For instance, Testing OutputWrapper initializes stream Testing … |
|
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E126 continuation line over-indented for hanging indent |
![]() |
|
F401 'rbtools.commands.JSONWrapper' imported but unused |
![]() |
|
F821 undefined name 'JSONOutput' |
![]() |
rbtools/commands/__init__.py (Diff revision 1) |
---|
Summary: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
Fixing Review Bot issues and multiline string formatting
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+496 -344) |
Commits: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+506 -352) |
Added new test for Outputwrapper's new_line() that ensures output stream receives new line character
Commits: |
|
||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+527 -355) |
rbtools/commands/patch.py (Diff revision 4) |
---|
For consistency with the other changes, should this maybe instead be self.stdout.new_line() ?
rbtools/commands/post.py (Diff revision 4) |
---|
Again for consistency, maybe this should be self.stdout.new_line() ?
Replaced self.stdout.write("\n") with self.stdout.new_line()
Commits: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+543 -369) |
Commits: |
|
||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+542 -370) |
Changed status from work in progress to in review
Summary: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Commits: |
|
||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+570 -398) |
Commits: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+579 -405) |
rbtools/commands/tests/__init__.py (Diff revision 8) |
---|
All test cases should be in files called
test_X
, not the__init__.py
. In this case I think you could probably add your new test class to the existingtest_main.py
file.This class should also probably be called
OutputWrapperTests
.
rbtools/commands/tests/__init__.py (Diff revision 8) |
---|
A few changes in here:
- The file needs a module docstring
- The first thing after that docstring should be
from __future__ import unicode_literals
- Imports should be structured in three groups: standard library, third-party libraries, and then rbtools. In each group we alphabetize the imports. So these should look like:
import sys from kgb import SpyAgency from rbtools.commands import OutputWrapper from rbtools.utils.testbase import RBTestBase
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+615 -449) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+617 -449) |
rbtools/commands/__init__.py (Diff revision 10) |
---|
Let's add another newline in here, before the format operation. Otherwise it's sometimes easy to miss when scanning quickly through the code.
self.stdout.write('Please log in to the Review Board server at ' '%s.' % urlparse(uri)[1])
rbtools/commands/info.py (Diff revision 10) |
---|
The second line here needs to be indented a bit more:
% (review_request.submitter.fullname or review_request.submitter.username))
rbtools/commands/land.py (Diff revision 10) |
---|
When we wrap a string, we put the space at the end of the first line. Let's also put the format parameters on their own line:
self.stdout.write('Recursively landing dependencies of ' 'review request %s.' % review_request_id)
rbtools/commands/patch.py (Diff revision 10) |
---|
Please undo the changes here (so the space is at the end of the first line).
rbtools/commands/status_update.py (Diff revision 10) |
---|
The new formatting here is so much nicer! Good work.
rbtools/commands/tests/test_main.py (Diff revision 10) |
---|
One thing Christian just pointed out to me is that he's been moving to just
import kgb
, and then usingkgb.SpyAgency
where necessary. This makes it easier when we want to add things later like spy operations.
rbtools/commands/tests/test_main.py (Diff revision 10) |
---|
Please make sure this is sorted alphabetically.
rbtools/commands/tests/test_main.py (Diff revision 10) |
---|
This isn't a good description. Let's say "Unit tests for command OutputWrapper"
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+652 -458) |
rbtools/commands/__init__.py (Diff revision 11) |
---|
Docstrings need to be in the following form:
"""Single-line summary. Multi-line description. """
Along with this, for any newly-introduced classes (or new functions/attributes on existing classes), we want to include version information directly below the description, like so:
"""Single-line summary. Multi-line description. Version Added: 3.0 """
rbtools/commands/__init__.py (Diff revision 11) |
---|
Function docstrings must follow the same form, and specify any arguments, return values, and exceptions raised. Take a look at the docs for this on Notion.
We do have a lot of old docstrings in RBTools that don't conform to the modern standard, but if you grep around for "Args:" or "Returns:", you'll see examples.
This comment applies throughout the change.
rbtools/commands/__init__.py (Diff revision 11) |
---|
We expect a blank line between the end of a block and a new statememt, to help separate that out as a new chunk of code.
rbtools/commands/__init__.py (Diff revision 11) |
---|
There's some typos in these docstrings ("charater" here, "specifiy" in the class docstring). If your editor supports it, I recommend running a spell check.
rbtools/commands/__init__.py (Diff revision 11) |
---|
This docstring is old, and needs to be modernized. As a step toward this, since you're introducing some really important new attributes, let's begin documenting them.
To do this, add an
Attributes
section at the end of this docstring:"""... ... Attributes: stderr (OutputWrapper): Standard error output wrapper that subclasses must write to. stdout (OutputWrapper): ... ...
Attributes should be in alphabetical order.
rbtools/commands/__init__.py (Diff revision 11) |
---|
There's an inconsistency in variable names. You have
stderr_bytes
(plural) andstderr_byte
(singular).The plural form is what we want, I think.
Also, this if/else always sets both, so we have no reason to default to
None
above.
rbtools/commands/patch.py (Diff revision 11) |
---|
Here's an area where the new output wrappers solve a problem!
Not all diffs can be decoded as UTF-8, so we want to write as a byte string. This code here was using
sys.stdout.buffer.write
to let us do that on Python 3, and justprint()
on Python 2.The correct thing will be to replace this entire chunk of logic with a write to
self.stdout_bytes
.Make sure, along with replacing
sys.stdout
orsys.stderr
.
rbtools/commands/setup_repo.py (Diff revision 11) |
---|
The string is a parameter to
textwrap.fill()
, rather than a second parameter tostdout.write
. Since we're wrapping to the following line to give us room for the text, we need to keep it 4 spaces indented from the start of the main statement. In order words, what we had before was correct.
rbtools/commands/setup_repo.py (Diff revision 11) |
---|
To keep lint checkers from complaining about indentations being multiples of 4, this should actually be in the same form we had before. The parameters indent to 4 spaces relative to the statement, allowing dictionary keys to indent 4 spaces from that. So you can revert these indentation changes, and keep the string on the second line.
rbtools/commands/tests/test_main.py (Diff revision 11) |
---|
This is a class docstring, so it needs to have a trailing period.
rbtools/commands/tests/test_main.py (Diff revision 11) |
---|
All unit test docstrings should be in the form of
Testing <Thing> <condition>
.For instance,
Testing OutputWrapper initializes stream
Testing OutputWrapper.write with ...
Testing OutputWrapper.new_line
etc.
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+752 -500) |
rbtools/commands/setup_repo.py (Diff revision 12) |
---|
E126 continuation line over-indented for hanging indent
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+765 -515) |
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+465 -99) |
rbtools/commands/tests/test_main.py (Diff revision 14) |
---|
F401 'rbtools.commands.JSONWrapper' imported but unused
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+466 -100) |
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 16 (+765 -515) |