flake8
-
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 1) Show all issues -
reviewboard/webapi/tests/test_review_comment.py (Diff revision 1) F841 local variable 'rsp' is assigned to but never used
Review Request #10190 — Created Oct. 19, 2018 and submitted
Information | |
---|---|
brennie | |
Review Board | |
release-4.0.x | |
|
|
10242 | |
b959551... | |
Reviewers | |
reviewboard | |
The
ReviewDiffCommentResource
is now capable of creating comments on
FileDiffs with a base FileDiff ID. This allows us to comment on
FileDiffs in commit ranges and keep the context of the base FileDiff.
Ran unit tests.
Description | From | Last Updated |
---|---|---|
E128 continuation line under-indented for visual indent |
![]() |
|
F841 local variable 'rsp' is assigned to but never used |
![]() |
|
Seems like this should be "The ID fo the base FileDiff" or "The base FileDiff ID" |
|
|
This would be more concise as: return (self.extra_data and self.extra_data.get(...)) |
|
|
Mind putting :term: around "cumulative diff?" We'll still need to define it, but it's such an insider term that I … |
|
|
I don't understand what this means. |
|
|
We should handle the case of this not existing. I know it should exist, but we've seen cases like this … |
|
|
Alphabetical order. |
|
|
Same comments as above with the terms and referencing commit histories. |
|
|
This really should have a period at the end. I know it didn't before, but most of our errors do. … |
|
|
This gets pretty deeply nested, and the checking of base_filediff_id here and the setting in the else clause can initially … |
|
|
This should be one statement. |
|
|
Typo: "Comapare" -> "Compare" Also, I think the "with" before "response" shouldn't be there. |
|
|
Let's just compare for truthiness. This is a bit fragile otherwise. |
|
|
Missing a docstring. |
|
|
"commit history" |
|
|
E999 SyntaxError: invalid syntax |
![]() |
|
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
Let's use :term: here for "cumulative diff," since we now have it. |
|
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 1) |
---|
reviewboard/webapi/tests/test_review_comment.py (Diff revision 1) |
---|
F841 local variable 'rsp' is assigned to but never used
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+871 -41) |
reviewboard/reviews/models/diff_comment.py (Diff revision 2) |
---|
Seems like this should be "The ID fo the base FileDiff" or "The base FileDiff ID"
reviewboard/reviews/models/diff_comment.py (Diff revision 2) |
---|
This would be more concise as:
return (self.extra_data and self.extra_data.get(...))
reviewboard/webapi/resources/base_diff_comment.py (Diff revision 2) |
---|
Mind putting
:term:
around "cumulative diff?" We'll still need to define it, but it's such an insider term that I think we'll need to if we're going to at all reference it publicly.This should probably also mention that it's used for review requests with commit history.
reviewboard/webapi/resources/base_diff_comment.py (Diff revision 2) |
---|
I don't understand what this means.
reviewboard/webapi/resources/base_diff_comment.py (Diff revision 2) |
---|
We should handle the case of this not existing. I know it should exist, but we've seen cases like this before with corruption/hand-modified tables.
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2) |
---|
Same comments as above with the terms and referencing commit histories.
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2) |
---|
This really should have a period at the end. I know it didn't before, but most of our errors do.
All the ones below should as well.
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2) |
---|
This gets pretty deeply nested, and the checking of
base_filediff_id
here and the setting in theelse
clause can initially be confusing. How about:if filediff is None or not dvcs_feaure.is_enabled(request=request): base_filediff_id = None if base_filediff_id is not None: ...
reviewboard/webapi/tests/test_review_comment.py (Diff revision 2) |
---|
Typo: "Comapare" -> "Compare"
Also, I think the "with" before "response" shouldn't be there.
reviewboard/webapi/tests/test_review_comment.py (Diff revision 2) |
---|
Let's just compare for truthiness. This is a bit fragile otherwise.
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+843 -40) |
reviewboard/webapi/resources/base_diff_comment.py (Diff revision 3) |
---|
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+824 -40) |
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 4) |
---|
Let's use
:term:
here for "cumulative diff," since we now have it.
Addressed feedback
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+824 -40) |