Use the RB.DiffCommitListView for change descriptions

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

brennie
Review Board
release-4.0.x
10126
10128
a0f7c50...
reviewboard

The commit lists in change descriptions now use the
RB.DiffCommitListView so that their contents can be expanded/collapsed
as needed.

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

Description From Last Updated

F401 'reviewboard.diffviewer.commit_utils.diff_histories' imported but unused

reviewbotreviewbot

F811 redefinition of unused 'test_render_change_entry_html_expand' from line 364

reviewbotreviewbot

You can assign commits directly instead of having an otherwise unused variable.

chipx86chipx86

Should use keyword arguments for future compatibility.

chipx86chipx86

Missing :js:class:.

chipx86chipx86

Can this use the new serialize()?

chipx86chipx86

Alphabetical order.

chipx86chipx86

There should be two blank lines on both sides.

chipx86chipx86

No spaces immediately within {..}

chipx86chipx86

Can you update this to use the expanded form?

chipx86chipx86

Alphabetical order.

chipx86chipx86

Should use the expanded form.

chipx86chipx86

F821 undefined name 'commits'

reviewbotreviewbot

Rather than .update(), just set model_data['commits'] directly, since it's the only thing going in.

chipx86chipx86

Typo: "attribtues" -> "attributes"

chipx86chipx86

Same typo.

chipx86chipx86

Super nit, but can we swap these so it's alphabetical?

daviddavid

Doc comment?

daviddavid

I don't understand why you changed this to be a function. It seems like we could leave it as-is and ...

daviddavid

This needs a doc comment that explains why it's a function.

daviddavid

This needs to be documented in ModelAttributes above.

chipx86chipx86

Missing trailing comma.

chipx86chipx86

As in the other change, I'd prefer if we only added the colspan if needed.

chipx86chipx86

Let's set the list of commits directly into the context.

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

flake8

brennie
brennie
Review request changed
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 4)
     
     
     
     

    You can assign commits directly instead of having an otherwise unused variable.

    1. commits is used. It is used by _get_common_context.

    2. Oops, misread this.

  3. reviewboard/reviews/builtin_fields.py (Diff revision 4)
     
     
     

    Should use keyword arguments for future compatibility.

  4. reviewboard/reviews/detail.py (Diff revision 4)
     
     

    Missing :js:class:.

  5. reviewboard/reviews/detail.py (Diff revision 4)
     
     
     
     
     
     

    Can this use the new serialize()?

    1. This patch actually comes before /r/10128/ in my tree. Missed the depends-on.

    2. Oops no I didn't. /r/10128 correctly depends on this change, which fixes this.

  6. Alphabetical order.

  7. There should be two blank lines on both sides.

  8. No spaces immediately within {..}

  9. Can you update this to use the expanded form?

  10. Alphabetical order.

  11. 
      
brennie
Review request changed
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/detail.py (Diff revision 6)
     
     
     

    Rather than .update(), just set model_data['commits'] directly, since it's the only thing going in.

  3. Typo: "attribtues" -> "attributes"

  4. 
      
brennie
david
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
     

    Super nit, but can we swap these so it's alphabetical?

  3. I don't understand why you changed this to be a function. It seems like we could leave it as-is and then do something that looks very much like it for ChangeEntry's defaults

    1. It is returning mutable state (the arrays), which would otherwise be the same arrays each time.

  4. 
      
brennie
david
  1. 
      
  2. This needs a doc comment that explains why it's a function.

  3. 
      
brennie
chipx86
  1. 
      
  2. This needs to be documented in ModelAttributes above.

  3. As in the other change, I'd prefer if we only added the colspan if needed.

  4. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 10)
     
     
     
     

    Let's set the list of commits directly into the context.

    1. I disagree. Let's set the list of commits directly into the context.

    2. And don't make me leave this same incorrect review again!

    3. https://reviews.reviewboard.org/r/10127/#comment45452

  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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