Display the selected revision's commits in the diffviewer

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

Information

Review Board
release-4.0.x
c643e35...

Reviewers

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.

Description From Last Updated

Typo in the description: "commist" -> "commit"

chipx86chipx86

F401 'reviewboard.diffviewer.features.dvcs_feature' imported but unused

reviewbotreviewbot

Col: 27 Missing semicolon.

reviewbotreviewbot

Col: 14 'testRows' was used before it was defined.

reviewbotreviewbot

Leftover debug output.

chipx86chipx86

This can easily be in alphabetical order. (While that doesn't always make things better in CSS -- width and height, …

chipx86chipx86

Swap these.

chipx86chipx86

Alphabetical order for these selectors.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing docs.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

interview? :)

chipx86chipx86

Wrong number of spaces for indentation.

chipx86chipx86

Extra blank line.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Can you be more explicit in the TODO?

chipx86chipx86

Can you split out the template parameters so it's consistent with other calls in the codebase and easier to update? …

chipx86chipx86

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

chipx86chipx86

Here, too.

chipx86chipx86

Technically this is a jQuery.Event. Same below.

chipx86chipx86

Extra blank line.

chipx86chipx86

Extra blank line.

chipx86chipx86

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

chipx86chipx86

Blank line between these.

chipx86chipx86

No blank line here.

chipx86chipx86

function(), not () =>. Same with all others in the tests in this change.

chipx86chipx86

No need for </div>.

chipx86chipx86

No need to define to null.

chipx86chipx86

Can you use the explicit expanded form? Same below.

chipx86chipx86

No spaces within the {}. This file is inconsistent on this. Same with all below.

chipx86chipx86

Missing trailing comma.

chipx86chipx86

Missing trailing comma.

chipx86chipx86

No need for the default value.

chipx86chipx86

Comma before "if any."

chipx86chipx86

Missing docs.

chipx86chipx86

If we can avoid this form, we should. It ends up being unnecessarily expensive, since it doesn't really add anything …

chipx86chipx86

Can you put this in alphabetical order?

chipx86chipx86

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

chipx86chipx86

Two blank lines on either side.

chipx86chipx86

Should be Model Attributes.

chipx86chipx86

boolean

chipx86chipx86

Model Attribute.

chipx86chipx86

Can you explain what needs to be kept in sync?

chipx86chipx86

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

chipx86chipx86

Kinda looks wrong. Let's do: $content = $(tableTemplate({ ... })) .append(...)

chipx86chipx86

boolean

chipx86chipx86

Typo: "attribtues" -> "attributes".

chipx86chipx86

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

chipx86chipx86

Alphabetical order.

chipx86chipx86

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

chipx86chipx86

Still need two blank lines.

chipx86chipx86

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

chipx86chipx86

No spaces immediately within {..}

chipx86chipx86

No spaces immediately within {..}

chipx86chipx86

No spaces immediately within {..}

chipx86chipx86

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

chipx86chipx86

Alphabetical order.

chipx86chipx86

Alphabetical order.

chipx86chipx86

Can you check isEmpty() instead?

chipx86chipx86

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Can we put the expression in parens?

daviddavid

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

daviddavid

Same here.

daviddavid

We should probably also do e.stopPropagation()

daviddavid

We should probably also do e.stopPropagation()

daviddavid

Needs "Args:" now.

daviddavid
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...