Display the selected revision's commits in the diffviewer

Review Request #10125 — Created Aug. 24, 2018 and submitted

brennie
Review Board
release-4.0.x
10094
10126
c643e35...
reviewboard

The diffviewer now displays the list of commit messages and authors and
they are updated when the selected revision changes. Currently the
commit list doesn't support interdiffs and an error message is
displayed instead. This will be addressed in a future patch.

  • Ran unit tests.
  • Ran JS tests.
Loading file attachments...

  • 0
  • 0
  • 68
  • 2
  • 70
Description From Last Updated
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

brennie
brennie
chipx86
  1. 
      
  2. Typo in the description: "commist" -> "commit"

  3. reviewboard/reviews/tests/test_builtin_fields.py (Diff revision 3)
     
     
     
     

    Leftover debug output.

  4. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3)
     
     
     
     

    This can easily be in alphabetical order. (While that doesn't always make things better in CSS -- width and height, for instance, should usually go together -- it's worth doing here.)

  5. Swap these.

  6. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3)
     
     
     
     
     
     
     
     
     

    Alphabetical order for these selectors.

  7. This is using a trailing comma, but this file isn't a .es6.js. Can you rename the file?

  8. Can you add a constant for the max length, and then compute the two numbers from that?

  9. I'd prefer to be explicit and not use the shorthand. Especially when we're having to be explicit for other values in the dictionary.

  10. Missing a trailing comma.

  11. reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js (Diff revision 3)
     
     
     
     
     
     
     
     
     

    Wrong number of spaces for indentation.

  12. Can you be more explicit in the TODO?

  13. Can you split out the template parameters so it's consistent with other calls in the codebase and easier to update? One line per variable.

  14. As above, I'd prefer being explicit with a key: value form.

  15. Technically this is a jQuery.Event.

    Same below.

  16. We need to explicitly use function() with Jasmine instead of =>, as this matters.

  17. function(), not () =>.

    Same with all others in the tests in this change.

  18. Can you use the explicit expanded form?

    Same below.

  19. No spaces within the {}. This file is inconsistent on this.

    Same with all below.

  20. If we can avoid this form, we should. It ends up being unnecessarily expensive, since it doesn't really add anything beyond what we could do before and just expands to a bunch of otherwise useless code (in this usage).

  21. reviewboard/staticbundles.py (Diff revision 3)
     
     
     

    Can you put this in alphabetical order?

  22. In sync how?

    Given the length and need for more details, this can use {% comment %}...{% endcomment %}.

  23. 
      
brennie
chipx86
  1. 
      
  2. Two blank lines on either side.

  3. Should be Model Attributes.

  4. Can you explain what needs to be kept in sync?

  5. Multi-line comments should be in /* .. */ format.

  6. Kinda looks wrong. Let's do:

    $content =
        $(tableTemplate({
            ...
        }))
        .append(...)
    
  7. Typo: "attribtues" -> "attributes".

  8. Needs to be .call instead of .apply if not passing a list of arguments.

  9. Alphabetical order.

  10. 
      
brennie
chipx86
  1. The code is good. There's just a few inconsistencies within the new code that I want to address.

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

    This defaults to an empty list, but the other view defaults to None. We should be consistent to avoid issues down the road.

  3. Still need two blank lines.

  4. Let's be explicit, especially since we are in the two cases above.

  5. No spaces immediately within {..}

  6. 
      
brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. This can just be <p>.

  3. Can you check isEmpty() instead?

  4. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These templates are all using 2 spaces for indentation in the HTML, but they should use 1 like all other HTML-based templates.

  3. 
      
brennie
brennie
brennie
brennie
Review request changed
brennie
Review request changed
brennie
Review request changed
brennie
david
  1. 
      
  2. Can we put the expression in parens?

  3. You don't need to use the $() around headerTemplate--you can just append the HTML.

  4. We should probably also do e.stopPropagation()

  5. We should probably also do e.stopPropagation()

  6. 
      
brennie
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (2437358)
Loading...