Add typing structures for the diff viewer context.

Review Request #13666 — Created March 25, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

The context that's assembled and used to render the diff viewer page
(both the HTML-based UI and the diff context API endpoint) has grown
organically since the very earliest days of Review Board, and is a
pretty big hairy mess. In order to bring at least a little bit of sanity
to it, this change adds typing structures so that type checkers can
verify that we're always assigning things correctly.

The only actual change to the content of the context is that we can no
longer end up with non-bool types for the is_draft_diff and
is_draft_interdiff fields.

Ran unit tests.

Summary ID
Add typing structures for the diff viewer context.
The context that's assembled and used to render the diff viewer page (both the HTML-based UI and the diff context API endpoint) has grown organically since the very earliest days of Review Board, and is a pretty big hairy mess. In order to bring at least a little bit of sanity to it, this change adds typing structures so that type checkers can verify that we're always assigning things correctly. The only actual change to the content of the context is that we can no longer end up with non-`bool` types for the `is_draft_diff` and `is_draft_interdiff` fields. Testing Done: - Ran unit tests.
eaf2f91109549ee9bfe835fbac9bc42b86f397aa
Description From Last Updated

Missing Returns.

chipx86chipx86

Might be a good time to deprecate non-keyword-only arguments.

chipx86chipx86

Can you add Args for these?

chipx86chipx86

If we're not documenting these, we don't need blank lines between them.

chipx86chipx86

Blank line between these.

chipx86chipx86

These can be assertFalse.

chipx86chipx86
chipx86
  1. Overall looks good, but I have two broad thoughts:

    1. Discussion point: I wonder if we should consider putting at least some of these serialized types into a module that can own them. This wouldn't make sense everywhere, but might help keep some things organized? It'd be a fine line, so I don't really have anything more to offer on this.

    2. I think we probably should document these serialized types, if they're intended to be consumed outside of the module. I know we talked about this in the context of TypeScript, consuming APIs, but when we're providing them in a situation where changes may impact a caller's use, we should document them so there's a clear path to versioning and deprecating.

      1. Hmm. I'll have to think about this. For now I think keeping those definitions close to the views that use them is best.
      2. Adding docs isn't too much of a burden, though the amount that we "use" these is pretty limited outside of the code that's changed in here, plus the templates that render the pages.
  2. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
     
     
     
     
     
     
     

    Missing Returns.

  3. reviewboard/reviews/context.py (Diff revision 1)
     
     
     

    Might be a good time to deprecate non-keyword-only arguments.

  4. reviewboard/reviews/context.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     

    Can you add Args for these?

  5. reviewboard/reviews/models/review_request.py (Diff revision 1)
     
     
     
     
     
     

    If we're not documenting these, we don't need blank lines between them.

  6. reviewboard/reviews/views/diffviewer.py (Diff revision 1)
     
     
     

    Blank line between these.

  7. These can be assertFalse.

  8. 
      
david
maubin
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (73c3882)
Loading...