Switch between selected commit revisions shown in the diffviewer

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

brennie
Review Board
release-4.0.x
10132, 10231, 10186
10190, 10233, 10191
d543ed9...
reviewboard

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.
  • 0
  • 0
  • 27
  • 0
  • 27
Description From Last Updated
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

Diff:

Revision 2 (+1130 -236)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

brennie
david
  1. 
      
  2. 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)
     
     

    request -> requested?

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

    request -> requested?

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

    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)
     
     

    Too many blank lines now.

  7. Missing the explanations here.

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

  9. 
      
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/commit_utils.py (Diff revision 6)
     
     
     
     
     

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

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

    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)
     
     
     
     
     

    Too many blank lines.

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

    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. Can you put parens around the expression?

  7. reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These are indented too far.

  8. 
      
brennie
chipx86
  1. 
      
  2. These should all align.

  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (1f60295)
Loading...