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

  • 0
  • 0
  • 20
  • 4
  • 24
Description From Last Updated
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...