Add typing and clean up code in reviewboard.reviews.detail.

Review Request #14309 — Created Jan. 24, 2025 and updated

Information

Review Board
release-7.1.x

Reviewers

This introduces type hints all throughout the code that builds the page
data for the review request page. There's a lot of complexity in here
with chances for things to go wrong (such as performance problems, which
this change will help facilitate fixing), so having thorough typing can
help pinpoint possible problems.

All attributes for ReviewRequestPageData and the entries are now typed
and documented, and all code interfacing with these utilize the right
types. There were a few places where we made some unsafe assumptions,
and these have been addressed.

Some of the code has been cleaned up to follow a pattern of building
data structures and then setting them on the class, rather than
re-accessing the data structure multiple times in loops. This is part
optimization, part type safety (we're using Mapping instead of dict,
for instance, to help avoid state pollution), and part preparation for
the round of performance fixes.

There are many places where we're doing things we probably shouldn't be
doing. For instance, we set a lot of custom state on models, for use at
entry and render times. Type hints rightly complain about these, so if
we ever decide to revisit some of these operations, they'll be readily
apparent.

All unit tests pass.

Tested against a series of local, heavily-populated review requests.
I didn't notice any regressions.

Compared the SQL statements before and after this change using
Django Debug Toolbar. Didn't see any regressions in queries.

Summary ID
Add typing and clean up code in reviewboard.reviews.detail.
This introduces type hints all throughout the code that builds the page data for the review request page. There's a lot of complexity in here with chances for things to go wrong (such as performance problems, which this change will help facilitate fixing), so having thorough typing can help pinpoint possible problems. All attributes for `ReviewRequestPageData` and the entries are now typed and documented, and all code interfacing with these utilize the right types. There were a few places where we made some unsafe assumptions, and these have been addressed. Some of the code has been cleaned up to follow a pattern of building data structures and then setting them on the class, rather than re-accessing the data structure multiple times in loops. This is part optimization, part type safety (we're using `Mapping` instead of `dict`, for instance, to help avoid state pollution), and part preparation for the round of performance fixes. There are many places where we're doing things we probably shouldn't be doing. For instance, we set a lot of custom state on models, for use at entry and render times. Type hints rightly complain about these, so if we ever decide to revisit some of these operations, they'll be readily apparent.
8db9a0f41fb84c8c6c8da790703f6fc9da6a924d
Description From Last Updated

We're switching the definition here from defaultdict to list, but then we reassign back to a defaultdict later. Can we …

daviddavid
There are no open issues
chipx86
Review request changed
Testing Done:
  +

All unit tests pass.

  +
  +

Tested against a series of local, heavily-populated review requests.

  + I didn't notice any regressions.

  +
  +

Compared the SQL statements before and after this change using

  + Django Debug Toolbar. Didn't see any regressions in queries.

david
  1. 
      
  2. reviewboard/reviews/detail.py (Diff revision 1)
     
     
     
    Show all issues

    We're switching the definition here from defaultdict to list, but then we reassign back to a defaultdict later. Can we keep this consistent?

    1. It's defined as a Mapping, defaulting to an empty dict. We build up a defaultdict of items, and then assign that as the new Mapping. I don't see where this is being typed or set as a list.

  3. 
      
david
  1. Ship It!
  2. 
      
maubin
  1. Ship It!
  2. 
      
Loading...