flake8
failed.
JSHint
passed.
flake8
-
reviewboard/diffviewer/chunk_generator.py (Diff revision 1) Show all issues -
-
-
Show cumulative diffs in the diffviewer
Review Request #10123 — Created Aug. 21, 2018 and submitted
| Information | |
|---|---|
| brennie | |
| Review Board | |
| release-4.0.x | |
|
|
|
| 10124 | |
| 7e6d551... | |
| Reviewers | |
| reviewboard | |
Previously,
FileDiffsfor review requests with history would not show
cumulatively. That is, you would see only the result of applying that
FileDiff, not the result of applying the entire stack ofFileDiffs
in the history graph.We now compute the entire file history, including deletions, and use
that to compute the cumulative diff. In other words, for a review
request with commitsA -> B -> Cwhere all three commits modify the
same file, previously you would see the result ofA,B, andC
independently. Now the diffviewer showsA,A..B, andA..C.Files are still repeated (as indicated above) so this is not yet a true
cumulative diff. A future patch will update the diffviewer to only show
the leafFileDiffsin the history graph.
- Ran unit tests.
- Uploaded a multi-commit review request and looked at the diff. I
observed cumulative (but repeated) diffs.
| Description | From | Last Updated |
|---|---|---|
|
E501 line too long (86 > 79 characters) |
 reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
E501 line too long (91 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
F811 redefinition of unused 'setUp' from line 2822 |
reviewbot
|
|
|
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
Can you explicitly compare against 0? |
 chipx86
|
|
|
One blank line. |
chipx86
|
|
|
Can you move the paren for the base_filediff stuff onto the next line, like: (newfile or (base_filediff is not None … |
chipx86
|
|
|
You'll need to import range from six, or we get a list here on Python 2. |
chipx86
|
|
|
Missing docstring. |
chipx86
|
|
|
Missing docstring. |
chipx86
|
|
|
Too many blank lines. |
chipx86
|
|
|
Too many blank lines. |
chipx86
|
|
|
We should compare in the returned order, not as a set, to catch future discrepencies. |
chipx86
|
|
|
Since we're looping and asserting stuff in each loop, we should print out something to help identify the diff_file we're … |
chipx86
|
|
|
This is indented 1 too many spaces. |
 david
|
|
|
I personally find it more readable to do for i in reversed(range(len(ancestor_ids))). No biggie though. |
david
|
Testing Done: |
|
|---|
Change Summary:
Better architecture.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+290 -76) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+290 -76) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Summary: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||
Commit: |
|
|||||||||
Diff: |
Revision 4 (+586 -234) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4) F401 'reviewboard.diffviewer.models.DiffSet' imported but unused
-
-
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4) F811 redefinition of unused 'setUp' from line 2822
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+585 -238) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) F401 'reviewboard.diffviewer.models.DiffSet' imported but unused
-
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+582 -237) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+581 -237) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Rebase
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+577 -233) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+578 -234) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Add more unit tests
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+635 -234) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 10) Can you move the paren for the
base_filediffstuff onto the next line, like:(newfile or (base_filediff is not None and ... ...) -
reviewboard/diffviewer/models/filediff.py (Diff revision 10) You'll need to import
rangefromsix, or we get a list here on Python 2. -
-
-
-
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10) We should compare in the returned order, not as a set, to catch future discrepencies.
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10) Since we're looping and asserting stuff in each loop, we should print out something to help identify the
diff_filewe're working on in case of assertion.
Change Summary:
Addressed feedback.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+643 -235) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 12 (+643 -235) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
reviewboard/diffviewer/models/filediff.py (Diff revision 12) I personally find it more readable to do
for i in reversed(range(len(ancestor_ids))). No biggie though.