flake8
-
reviewboard/testing/testcase.py (Diff revision 1) Show all issues
Review Request #9757 — Created March 7, 2018 and submitted
Information | |
---|---|
brennie | |
Review Board | |
release-4.0.x | |
|
|
9852 | |
78fba15... | |
Reviewers | |
reviewboard | |
The
FileDiff
list resource now supports filtering by a commit ID via
passing the?commit-id=<id>
query parameter. Doing so will limit the
results toFileDiff
objects who are associated with that commit.
Passing an invalid commit ID will result in no results being returned.
Ran unit tests.
Description | From | Last Updated |
---|---|---|
E501 line too long (87 > 79 characters) |
![]() |
|
Can you expand this docstring for arguments? |
|
|
HttpRequest |
|
|
Double backticks here. |
|
|
HttpRequest |
|
|
Instead of this method, which must be opted into, I think I'd feel more comfortable if we inversed how these … |
|
|
Too many blank lines. |
|
|
Too many blank lines. |
|
|
Diff contents should always be binary. This function should assert this. |
|
|
Same here. |
|
|
Should be a :py:class: reference. |
|
|
Missing "Returns". |
|
|
This should ideally be consistent in implicit vs. explicit comparisons. |
|
|
Most docs use "The HTTP request from the client." |
|
|
Missing a description. |
|
|
Same description as above. |
|
|
Do we want to allow for empty strings? Maybe just check truthiness? |
|
|
Same as above. |
|
|
Missing a description. |
|
|
Been noticing lately that your test functions are all backwards. The basic case should come first. The variations come after. … |
|
|
Can you explicitly say "with commit-id=", so it's clear what this is testing? |
|
|
Here, too. |
|
|
Should test that the results are what we expect. |
|
|
All the same comments from the previous file, regarding docstrings and ordering, apply here as well. |
|
|
Should check the results. |
|
|
F841 local variable 'pk' is assigned to but never used |
![]() |
|
E225 missing whitespace around operator |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
F841 local variable 'pk' is assigned to but never used |
![]() |
|
E225 missing whitespace around operator |
![]() |
|
"review request revision" is weird |
|
|
This isn't something the review request resource cares about. |
|
|
Doesn't this break? It seems like we're no longer limiting the results to the correct diff revision. |
|
|
This is incorrect, because this method ignores the diffset revision (which is filtered for in the caller). Can we just … |
|
|
"review request revision" isn't a thing |
|
|
Missing a "the". |
|
|
This should either be an assert statement or we should raise something like a ValueError. I'd prefer the latter. |
|
|
Is the second part of this logic correct? That should trigger if neither is provided (which the assertion says is … |
|
|
This isn't in the function signature. It can be removed here. |
|
Linting.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+141 -3) |
Add support to the draft filediff resource
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+253 -5) |
reviewboard/webapi/resources/filediff.py (Diff revision 3) |
---|
Instead of this method, which must be opted into, I think I'd feel more comfortable if we inversed how these querysets were built.
get_queryset()
should build the initial queryset that handlesdiffset__revision
(which is consistent between the implementations) and the commit filtering.It would then call a method defined by the class/subclass for filtering the review request association, like
get_diffset_query
. That handles thediffset__history__review_request=
here, and thediffset__review_request_draft=
inDraftFileDiffResource
.This simplifies implementations and keeps the control in the base
get_queryset
.
Addressed Christian's issues.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+289 -9) |
reviewboard/testing/testcase.py (Diff revision 4) |
---|
Diff contents should always be binary.
This function should assert this.
reviewboard/testing/testcase.py (Diff revision 4) |
---|
This should ideally be consistent in implicit vs. explicit comparisons.
reviewboard/webapi/resources/draft_filediff.py (Diff revision 4) |
---|
Most docs use "The HTTP request from the client."
reviewboard/webapi/resources/filediff.py (Diff revision 4) |
---|
Do we want to allow for empty strings? Maybe just check truthiness?
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 4) |
---|
Been noticing lately that your test functions are all backwards. The basic case should come first. The variations come after. Can you reorder these to be consistent with all our other tests?
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 4) |
---|
Can you explicitly say "with commit-id=", so it's clear what this is testing?
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 4) |
---|
Should test that the results are what we expect.
reviewboard/webapi/tests/test_filediff.py (Diff revision 4) |
---|
All the same comments from the previous file, regarding docstrings and ordering, apply here as well.
Addressed Christian's feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+317 -9) |
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 5) |
---|
F841 local variable 'pk' is assigned to but never used
reviewboard/webapi/tests/test_draft_filediff.py (Diff revision 5) |
---|
E225 missing whitespace around operator
reviewboard/webapi/tests/test_filediff.py (Diff revision 5) |
---|
F841 local variable 'pk' is assigned to but never used
Fix accidental paste issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+316 -9) |
Rebasing.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+316 -9) |
reviewboard/webapi/resources/filediff.py (Diff revision 7) |
---|
This isn't something the review request resource cares about.
reviewboard/webapi/resources/filediff.py (Diff revision 7) |
---|
Doesn't this break? It seems like we're no longer limiting the results to the correct diff revision.
Address David's feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+316 -9) |
Add missing local changes.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+316 -10) |
reviewboard/webapi/resources/filediff.py (Diff revision 9) |
---|
This is incorrect, because this method ignores the diffset revision (which is filtered for in the caller).
Can we just remove the
diff_revision
parameter entirely from this (since it's not used) and fix the docstring here to explain that it's returning all diffsets linked to the review request?
Addressed David's feedback
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+312 -10) |
reviewboard/testing/testcase.py (Diff revision 10) |
---|
This should either be an
assert
statement or we should raise something like aValueError
. I'd prefer the latter.
reviewboard/testing/testcase.py (Diff revision 10) |
---|
Is the second part of this logic correct? That should trigger if neither is provided (which the assertion says is okay), but won't trigger if only one is provided. Seems this would be more correct/readable:
if ((committer_name or commiter_email) and not (committer_name and committer_email)):
>>> ((True or False) and not (True and False)) True >>> ((False or True) and not (False and True)) True >>> ((True or True) and not (True and True)) False >>> ((False or False) and not (False and False)) False
Unit tests need to be fleshed out for all conditions.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+312 -10) |
reviewboard/webapi/resources/filediff.py (Diff revision 11) |
---|
This isn't in the function signature. It can be removed here.
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+309 -10) |