flake8
-
rbtools/utils/checks.py (Diff revision 1) Show all issues -
Review Request #11196 — Created Sept. 21, 2020 and submitted
Information | |
---|---|
ruonanjia | |
RBTools | |
master | |
Reviewers | |
rbtools, students | |
When pyenv is in use and the needed executable is only available in a
different Python version, pyenv intercepts the call and will print
an error/return 127.When running bzr help under these conditions, we see this:
pyenv: bzr: command not found
The `bzr' command exists in these Python versions:
2.7.14
Tested 'rbt post' manually in my local integrations repo with the changes.
Summary | Author |
---|---|
ruonan |
Description | From | Last Updated |
---|---|---|
So the generic "error code 127" might indicate a lot of different things. I think we have a couple options … |
|
|
For description and testing done, please make sure these wrap to 70 columns. We also want to make sure that … |
|
|
I think we need to add some more explanation in the description as well as some comments in the code … |
|
|
Your summary/description text is OK but please fix capitalization and punctuation. It might also be nice to put the error … |
|
|
F841 local variable 'streamdata' is assigned to but never used |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
|
Please make sure that sentences start with capital letters and end in periods. This can also be wrapped a little … |
|
|
The args to Popen need to be indented 4 more spaces to account for the new text you added. |
|
|
Instead of decoding to a unicode object, we can just look for the binary text. Please also use single quotes … |
|
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+14 -4) |
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+14 -4) |
So the generic "error code 127" might indicate a lot of different things. I think we have a couple options here:
- Treat any positive error code as an error. Theoretically this should be fine, but we would need to look at the various callers of
check_install
and verify that none of the things we're executing can return non-zero errors when it's actually a success case.- Keep the check for error 127, but additionally check the stderr output from the command to look for "command not found".
For description and testing done, please make sure these wrap to 70 columns. We also want to make sure that capitalization and punctuation is correct and consistent. We always use either "RBTools" or "rbtools" (in most cases the former is best when talking about it as a generic product and the latter when referring to the repo or modules in the code).
I think we need to add some more explanation in the description as well as some comments in the code explaining what's going on--I had to go back to your messages in Slack to realize what the problem was because the idea that
bzr
was simultaneously not installed and also supposedly executing with an error was very confusing.Let's explain that when pyenv is in use, and the needed executable is only available in a different Python version, pyenv intercepts the call and will print an error/return 127. Please include this example output of the error in the description:
pyenv: bzr: command not found The `bzr' command exists in these Python versions: 2.7.14
rbtools/utils/checks.py (Diff revision 3) |
---|
A few comments on this code, just FYI since I think it'll change a fair bit if you're adding the check for the output.
First, you had changed the
communicate
line to avoid storing the output, but it still has a[0]
at the end--that could be removed and just have it bep.communicate()
.Second, when we have blocks, we like to put a blank line around them to make the code just a bit easier to read. Like so:
rc = p.returncode if rc === 127: ...
That said, any time there's a "if X return True else return False", it can be simplified into a one-liner:
return (p.returncode != 127)
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+27 -11) |
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+20 -4) |
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+22 -4) |
Summary: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
Description: |
|
---|
Your summary/description text is OK but please fix capitalization and punctuation. It might also be nice to put the error output there in a code block (triple backticks around it)
rbtools/utils/checks.py (Diff revision 6) |
---|
Please make sure that sentences start with capital letters and end in periods. This can also be wrapped a little bit tighter to the textwidth.
Description: |
|
---|
Description: |
|
---|
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+25 -7) |
rbtools/utils/checks.py (Diff revision 7) |
---|
The args to
Popen
need to be indented 4 more spaces to account for the new text you added.
rbtools/utils/checks.py (Diff revision 7) |
---|
Instead of decoding to a
unicode
object, we can just look for the binary text. Please also use single quotes here:if b'command not found' in stderr:
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+38 -10) |