flake8
-
rbtools/api/resource.py (Diff revision 1) Show all issues -
-
rbtools/api/resource.py (Diff revision 1) F841 local variable 'query_args' is assigned to but never used
Review Request #9911 — Created May 4, 2018 and submitted
Information | |
---|---|
brennie | |
RBTools | |
master | |
9921 | |
d00641c... | |
Reviewers | |
rbtools | |
When the Review Board server has the
review_request.supports_history
capability, we will default to posting with history. Using history can
be forced with the new-H/--with-history
flag, which will cause an
error if the server does not support it.Traditional-style squashed review requests are still supported: they can
be posted with-S/--squash-history
.Review requests that were created with history will always be updated
with history and review requests not created with history will,
likewise, never be updated with history. Since review requests in
Review Board exist in one state or the other, attempting to post would
result in an error, so we do not let users attempt such posts.If the server does not support history, the
-S
and-H
options are
ignored.
Posted this review request.
Posted a review request with multiple commits to my devserver.
Description | From | Last Updated |
---|---|---|
E302 expected 2 blank lines, found 1 |
![]() |
|
F821 undefined name 'query_args' |
![]() |
|
F841 local variable 'query_args' is assigned to but never used |
![]() |
|
Docstring? |
|
|
XXX is meaningless without some kind of detail. Why is this comment here? |
|
|
typo: Attrirbutes |
|
|
commandline -> command line |
|
|
Creates or updates, and uploads -> Create or update, and upload |
|
|
Commandline -> Command line |
|
|
It was already like this, but commandline -> command line. |
|
|
Can we wrap the arg value in parens? |
|
|
commandline -> command line |
|
|
commandline -> command line |
|
|
Wrapping is a little weird here. |
|
|
E501 line too long (81 > 79 characters) |
![]() |
|
Can we call this supports_commit_history and the method below get_commit_history? Helps to namespace them so we're not having confusion down … |
|
|
It'd be nice to document what's in here/where it comes from. |
|
|
Byte string prefix? |
|
|
Can we simplify the language and say "cannot both be provided" instead of "mutually exclusive?" The latter's going to be … |
|
|
These would be nice to implement though. In fact, having history enabled by default and not allowing filtering is going … |
|
|
I don't really like doing this sort of thing. It'd be much nicer to have some proper checks with proper … |
|
|
Mind changing the wording here and below to properly say "review request URL" instead of "review URL?" |
|
|
This was correct before. |
|
|
Typo: "hvae" -> "have". |
|
|
Can you call this review_request_id and review_request_url? |
|
|
Too many "the review" parts in here. |
|
|
Can we make additional_fields a list of strings? Wishing we did that with only_links and only_fields in the API calls, … |
|
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
F821 undefined name 'review_url' |
![]() |
|
E203 whitespace before ',' |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
Two blank lines. |
|
|
Alphabetical order. |
|
|
Alphabetical order. |
|
|
This should mention the conditions for NotImplementedError and the formats (or references to something) for the revisions and results. |
|
|
Let's use constants for these, to help with readability and consistency. |
|
|
These should be the same import group. |
|
|
Blank line between these. |
|
|
These can start on the raise line. |
|
|
We're repeating tests unnecessarily. How about: if server_supports_history: if review_request: with_history = review_request.created_with_history if self.options.with_history: ... else: with_history = self._should_post_with_history(...) … |
|
|
This must be logging.warning. Can you test this case? |
|
|
bool |
|
|
You can do: only_fields = [ ... ] + additional_fields |
|
|
These can be combined into one statement. |
|
|
Are we checking for emptiness, or None? If the former, we don't even need the check, as it'll be handled … |
|
|
No trailing comma on the last argument. ) should be up against the last value. |
|
|
Let's set to None in an else. |
|
|
Use +=. It's faster. |
|
|
F821 undefined name 'Noen' |
![]() |
|
F821 undefined name 'Noen' |
![]() |
|
We don't have these for the others, unfortunately, but can you add a #: ... doc comment for this one? |
|
|
Should _FIELD_SEP be a byte string? We split a byte string using _NUL and then try to further split using … |
|
|
This wasn't updated to be get_commit_history. Make sure the latest iteration of changes has been fully tested. |
|
|
rbtools.clients.errors.SCMError. |
|
|
Need to add with_parent_diff here. |
|
|
These seem like artificial limitations that should probably not exist |
|
|
The long command-line arg is just --include |
|
|
The long command-line arg is just --exclude |
|
|
These error messages seem highly internal. Is this really what we want to show to a user? (When might this … |
|
|
Grammar here is weird. How about "One of "diff_history" or "squashed_diff" must be provided to "Post.post_request()." |
|
rbtools/api/resource.py (Diff revision 1) |
---|
rbtools/api/resource.py (Diff revision 1) |
---|
F841 local variable 'query_args' is assigned to but never used
Flake8 fixups
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+762 -190) |
rbtools/clients/git.py (Diff revision 2) |
---|
XXX is meaningless without some kind of detail. Why is this comment here?
rbtools/commands/post.py (Diff revision 2) |
---|
Creates or updates, and uploads -> Create or update, and upload
rbtools/commands/post.py (Diff revision 2) |
---|
It was already like this, but commandline -> command line.
Addressed David's issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+809 -192) |
Use TQDM to show a progress bar.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+815 -193) |
Include parent diff for first commit.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+818 -193) |
Send parent diff with all commits
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+817 -193) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+828 -193) |
rbtools/clients/__init__.py (Diff revision 7) |
---|
Can we call this
supports_commit_history
and the method belowget_commit_history
? Helps to namespace them so we're not having confusion down the road for any reason.
rbtools/clients/__init__.py (Diff revision 7) |
---|
It'd be nice to document what's in here/where it comes from.
rbtools/commands/post.py (Diff revision 7) |
---|
Can we simplify the language and say "cannot both be provided" instead of "mutually exclusive?" The latter's going to be easier for non-English speakers.
rbtools/commands/post.py (Diff revision 7) |
---|
These would be nice to implement though. In fact, having history enabled by default and not allowing filtering is going to break a whole lot of people. Is this a temporary restriction? What prevents us from being able to filter these?
rbtools/commands/post.py (Diff revision 7) |
---|
I don't really like doing this sort of thing. It'd be much nicer to have some proper checks with proper errors than an assertion that a lot of people working with RBTools wrappers and such are going to be confused by if they manage to hit it.
rbtools/commands/post.py (Diff revision 7) |
---|
Mind changing the wording here and below to properly say "review request URL" instead of "review URL?"
rbtools/commands/post.py (Diff revision 7) |
---|
Can you call this
review_request_id
andreview_request_url
?
rbtools/utils/review_request.py (Diff revision 7) |
---|
Can we make
additional_fields
a list of strings?Wishing we did that with
only_links
andonly_fields
in the API calls, but we can start making it right at least in this function.
addressed Christian's issues.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+851 -205) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+861 -206) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+861 -206) |
Fix posting with -u
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+861 -206) |
rbtools/commands/post.py (Diff revision 11) |
---|
We're repeating tests unnecessarily. How about:
if server_supports_history: if review_request: with_history = review_request.created_with_history if self.options.with_history: ... else: with_history = self._should_post_with_history(...) else: with_history = False
it's not as flat, so.. I dunno, tradeoffs. Throwing it out there.
rbtools/commands/post.py (Diff revision 11) |
---|
Are we checking for emptiness, or
None
? If the former, we don't even need the check, as it'll be handled for thefor
loop.
rbtools/commands/post.py (Diff revision 11) |
---|
No trailing comma on the last argument.
)
should be up against the last value.
rbtools/clients/git.py (Diff revision 11) |
---|
This should mention the conditions for
NotImplementedError
and the formats (or references to something) for the revisions and results.
rbtools/clients/git.py (Diff revision 11) |
---|
Let's use constants for these, to help with readability and consistency.
addressed Christian's issues.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+860 -206) |
More feedback addressed
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+896 -206) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+896 -206) |
rbtools/clients/__init__.py (Diff revision 14) |
---|
We don't have these for the others, unfortunately, but can you add a
#: ...
doc comment for this one?
rbtools/clients/git.py (Diff revision 14) |
---|
Should
_FIELD_SEP
be a byte string? We split a byte string using_NUL
and then try to further split using this.
rbtools/clients/git.py (Diff revision 14) |
---|
This wasn't updated to be
get_commit_history
.Make sure the latest iteration of changes has been fully tested.
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+897 -206) |
rbtools/commands/post.py (Diff revision 15) |
---|
These seem like artificial limitations that should probably not exist
rbtools/commands/post.py (Diff revision 15) |
---|
These error messages seem highly internal. Is this really what we want to show to a user? (When might this happen?)
rbtools/commands/post.py (Diff revision 15) |
---|
Grammar here is weird. How about "One of "diff_history" or "squashed_diff" must be provided to "Post.post_request()."
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+891 -206) |