Switch between selected commit revisions shown in the diffviewer

Review Request #10136 — Created Sept. 10, 2018 and submitted

Information

Review Board
release-4.0.x
d543ed9...

Reviewers

This patch builds upon the UI in /r/10132/ and implements the
functionality to render the actual content of the diff specified by the
selected commit range.

  • Ran unit tests.
  • Ran JS tests.
  • Viewed the diffs generated -- they looked correct.
Description From Last Updated

Would it be possible to split out the ES6 port part of this change into a separate review request?

daviddavid

W391 blank line at end of file

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F841 local variable 'diffset_commits' is assigned to but never used

reviewbotreviewbot

Col: 10 Missing semicolon.

reviewbotreviewbot

Col: 70 Missing semicolon.

reviewbotreviewbot

Col: 69 Missing semicolon.

reviewbotreviewbot

request -> requested?

daviddavid

request -> requested?

daviddavid

There's a lot of nesting and conditionals in here. Can we split this up to be more readable?

daviddavid

Too many blank lines now.

daviddavid

Needs a trailing comma.

daviddavid

Needs a trailing comma.

daviddavid

Missing the explanations here.

daviddavid

Needs a doc comment.

daviddavid

Looks to me like this returns a function, not an object.

daviddavid

Can we use a ~ for each class reference to trim the output a bit?

chipx86chipx86

Let's get rid of the double loop and need for constructing generators. What do you think of: needs_base_commit = base_commit_id …

chipx86chipx86

Too many blank lines.

chipx86chipx86

There's some redundancy in handling of missing data here. We know index is in GET, but we call .get() on …

chipx86chipx86

string

chipx86chipx86

string

chipx86chipx86

Can you put parens around the expression?

chipx86chipx86

These are indented too far.

chipx86chipx86

These should all align.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
Change Summary:

Rebased. Added js tests.

Summary:
WIP: Switch between selected commit revisions shown in the diffviewer
Switch between selected commit revisions shown in the diffviewer
Commit:
95605536806a8870ae11a35498bacb3a4f17d1d7
1eb2d33897c22087d203da658338f67cfc28b49b

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

brennie
david
  1. 
      
  2. Show all issues

    Would it be possible to split out the ES6 port part of this change into a separate review request?

    1. That is now https://reviews.reviewboard.org/r/10186/.

  3. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
    Show all issues

    request -> requested?

  4. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
    Show all issues

    request -> requested?

  5. reviewboard/diffviewer/commit_utils.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    There's a lot of nesting and conditionals in here.

    Can we split this up to be more readable?

  6. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues

    Too many blank lines now.

  7. Show all issues

    Needs a trailing comma.

  8. Show all issues

    Needs a trailing comma.

  9. Show all issues

    Missing the explanations here.

  10. Show all issues

    Needs a doc comment.

  11. Show all issues

    Looks to me like this returns a function, not an object.

  12. 
      
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/commit_utils.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Can we use a ~ for each class reference to trim the output a bit?

  3. reviewboard/diffviewer/commit_utils.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Let's get rid of the double loop and need for constructing generators. What do you think of:

    needs_base_commit = base_commit_id is not None
    needs_tip_commit = tip_commit_id is not None
    
    if needs_base_commit or needs_tip_commit:
        for commit in commits:
            if needs_base_commit and commit.pk == base_commit_id:
                base_commit = commit
                needs_base_commit = False
    
            if needs_tip_commit and commit.pk == tip_commit_id:
                tip_commit = commit
                needs_tip_commit = False
    
            if not needs_base_commit and not needs_tip_commit:
                break
    
    1. I think it can be more simplified since (a) commit PKs will never be None so we can always just jump to the comparison and (b) keeping the needs_{base|tip}_commit seems unnecessary since at most there are two commits we need to iterate over.

  4. reviewboard/diffviewer/views.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Too many blank lines.

  5. reviewboard/diffviewer/views.py (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    There's some redundancy in handling of missing data here. We know index is in GET, but we call .get() on it anyways. We're also then doing exception handling anywya. How about instead we do:

    try:
        diff_file['index'] = int(self.request.GET['index'])
    except (KeyError, ValueError):
        pass
    
  6. Show all issues

    string

  7. Show all issues

    string

  8. Show all issues

    Can you put parens around the expression?

  9. reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These are indented too far.

  10. 
      
brennie
chipx86
  1. 
      
  2. Show all issues

    These should all align.

  3. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (1f60295)