flake8
-
reviewboard/reviews/detail.py (Diff revision 1) Show all issues
Review Request #10127 — Created Aug. 24, 2018 and submitted
Information | |
---|---|
brennie | |
Review Board | |
release-4.0.x | |
|
|
10128 | |
a0f7c50... | |
Reviewers | |
reviewboard | |
The commit lists in change descriptions now use the
RB.DiffCommitListView
so that their contents can be expanded/collapsed
as needed.
Description | From | Last Updated |
---|---|---|
F401 'reviewboard.diffviewer.commit_utils.diff_histories' imported but unused |
![]() |
|
F811 redefinition of unused 'test_render_change_entry_html_expand' from line 364 |
![]() |
|
You can assign commits directly instead of having an otherwise unused variable. |
|
|
Should use keyword arguments for future compatibility. |
|
|
Missing :js:class:. |
|
|
Can this use the new serialize()? |
|
|
Alphabetical order. |
|
|
There should be two blank lines on both sides. |
|
|
No spaces immediately within {..} |
|
|
Can you update this to use the expanded form? |
|
|
Alphabetical order. |
|
|
Should use the expanded form. |
|
|
F821 undefined name 'commits' |
![]() |
|
Rather than .update(), just set model_data['commits'] directly, since it's the only thing going in. |
|
|
Typo: "attribtues" -> "attributes" |
|
|
Same typo. |
|
|
Super nit, but can we swap these so it's alphabetical? |
|
|
Doc comment? |
|
|
I don't understand why you changed this to be a function. It seems like we could leave it as-is and … |
|
|
This needs a doc comment that explains why it's a function. |
|
|
This needs to be documented in ModelAttributes above. |
|
|
Missing trailing comma. |
|
|
As in the other change, I'd prefer if we only added the colspan if needed. |
|
|
Let's set the list of commits directly into the context. |
|
Unit test fixes.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+405 -81) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+505 -86) |
reviewboard/reviews/tests/test_builtin_fields.py (Diff revision 3) |
---|
F811 redefinition of unused 'test_render_change_entry_html_expand' from line 364
Remove duplicate unit test?? git rerere is not without drawbacks...
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+408 -86) |
reviewboard/reviews/builtin_fields.py (Diff revision 4) |
---|
You can assign
commits
directly instead of having an otherwise unused variable.
reviewboard/reviews/builtin_fields.py (Diff revision 4) |
---|
Should use keyword arguments for future compatibility.
reviewboard/static/rb/js/reviewRequestPage/views/changeEntryView.es6.js (Diff revision 4) |
---|
Alphabetical order.
reviewboard/static/rb/js/reviewRequestPage/views/changeEntryView.es6.js (Diff revision 4) |
---|
Should use the expanded form.
Addressed feedback
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+408 -82) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+408 -81) |
reviewboard/reviews/detail.py (Diff revision 6) |
---|
Rather than
.update()
, just setmodel_data['commits']
directly, since it's the only thing going in.
Addressed feedback.
Depends On: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 7 (+407 -82) |
reviewboard/reviews/builtin_fields.py (Diff revision 7) |
---|
Super nit, but can we swap these so it's alphabetical?
I don't understand why you changed this to be a function. It seems like we could leave it as-is and then do something that looks very much like it for
ChangeEntry
'sdefaults
Addressed feedback.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+413 -81) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+424 -81) |
reviewboard/templates/reviews/changedesc_commit_list.html (Diff revision 9) |
---|
As in the other change, I'd prefer if we only added the
colspan
if needed.
Addressed feedback
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+429 -80) |
reviewboard/reviews/builtin_fields.py (Diff revision 10) |
---|
Let's set the list of commits directly into the context.