flake8
failed.
JSHint
passed.
flake8
-
reviewboard/reviews/detail.py (Diff revision 1) Show all issues
Use the RB.DiffCommitListView for change descriptions
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.DiffCommitListViewso that their contents can be expanded/collapsed
as needed.
- Ran JS tests.
- Ran unit tests.
| Description | From | Last Updated |
|---|---|---|
|
F401 'reviewboard.diffviewer.commit_utils.diff_histories' imported but unused |
 reviewbot
|
|
|
F811 redefinition of unused 'test_render_change_entry_html_expand' from line 364 |
reviewbot
|
|
|
You can assign commits directly instead of having an otherwise unused variable. |
 chipx86
|
|
|
Should use keyword arguments for future compatibility. |
chipx86
|
|
|
Missing :js:class:. |
chipx86
|
|
|
Can this use the new serialize()? |
chipx86
|
|
|
Alphabetical order. |
chipx86
|
|
|
There should be two blank lines on both sides. |
chipx86
|
|
|
No spaces immediately within {..} |
chipx86
|
|
|
Can you update this to use the expanded form? |
chipx86
|
|
|
Alphabetical order. |
chipx86
|
|
|
Should use the expanded form. |
chipx86
|
|
|
F821 undefined name 'commits' |
reviewbot
|
|
|
Rather than .update(), just set model_data['commits'] directly, since it's the only thing going in. |
chipx86
|
|
|
Typo: "attribtues" -> "attributes" |
chipx86
|
|
|
Same typo. |
chipx86
|
|
|
Super nit, but can we swap these so it's alphabetical? |
 david
|
|
|
Doc comment? |
david
|
|
|
I don't understand why you changed this to be a function. It seems like we could leave it as-is and … |
david
|
|
|
This needs a doc comment that explains why it's a function. |
david
|
|
|
This needs to be documented in ModelAttributes above. |
chipx86
|
|
|
Missing trailing comma. |
chipx86
|
|
|
As in the other change, I'd prefer if we only added the colspan if needed. |
chipx86
|
|
|
Let's set the list of commits directly into the context. |
chipx86
|
Change Summary:
Unit test fixes.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+405 -81) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+505 -86) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/reviews/tests/test_builtin_fields.py (Diff revision 3) F811 redefinition of unused 'test_render_change_entry_html_expand' from line 364
Change Summary:
Remove duplicate unit test?? git rerere is not without drawbacks...
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+408 -86) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/reviews/builtin_fields.py (Diff revision 4) You can assign
commitsdirectly 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.
Change Summary:
Addressed feedback
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+408 -82) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+408 -81) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/reviews/detail.py (Diff revision 6) Rather than
.update(), just setmodel_data['commits']directly, since it's the only thing going in. -
-
Change Summary:
Addressed feedback.
Depends On: |
|
||||
|---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 7 (+407 -82) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
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
Change Summary:
Addressed feedback.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+413 -81) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+424 -81) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
reviewboard/templates/reviews/changedesc_commit_list.html (Diff revision 9) As in the other change, I'd prefer if we only added the
colspanif needed.
Change Summary:
Addressed feedback
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+429 -80) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/reviews/builtin_fields.py (Diff revision 10) Let's set the list of commits directly into the context.