Add typing and missing documentation to review request fields.

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

Information

Review Board
release-7.1.x

Reviewers

The review request fields codebase is pretty crucial to Review Board,
and one of the primary extension interfaces that companies tend to
use and specialize.

In an effort to modernize fields and help reason about them in other
code that needs to interact with them, this change adds typing
throughout the fields codebases.

Fields are now generics, storing the type of data that can be
represented when loading and saving values.

All rendering functions now have return types of SafeString.

The base functions rendering or serializing change description state
work with Any as the type. This is because, while we have a
somewhat-standard format most fields use to record state in
ChangeDescription.fields_changed, this is entirely at the whim of the
implementation. As such, it's up to the field class to narrow the type
as needed and perform any checks and parsing required.

All unit tests passed.

Tested all the field types on a review request, and a wide variety of
change description renderings.

Summary ID
Add typing and missing documentation to review request fields.
The review request fields codebase is pretty crucial to Review Board, and one of the primary extension interfaces that companies tend to use and specialize. In an effort to modernize fields and help reason about them in other code that needs to interact with them, this change adds typing throughout the fields codebases. Fields are now generics, storing the type of data that can be represented when loading and saving values. All rendering functions now have return types of `SafeString`. The base functions rendering or serializing change description state work with `Any` as the type. This is because, while we have a somewhat-standard format most fields use to record state in `ChangeDescription.fields_changed`, this is entirely at the whim of the implementation. As such, it's up to the field class to narrow the type as needed and perform any checks and parsing required. In the process, several typing issues, bad assumptions, and incorrect documentation were found and fixed.
03186ee8fef188fc54d74ee130fb1a55ac448dfb
Description From Last Updated

Could say the User type here instead of object. And now that we've switched to importing from reviewboard.accounts.models.User, is that …

maubinmaubin

Group instead of object.

maubinmaubin

User instead of object.

maubinmaubin

You missed a super() here.

daviddavid

Forgot to add the type here.

maubinmaubin

And here.

maubinmaubin

Missing types on some of the attributes here.

maubinmaubin
Checks run (1 succeeded)
JSHint passed.
david
  1. 
      
  2. reviewboard/reviews/fields.py (Diff revision 1)
     
     
    Show all issues

    You missed a super() here.

  3. 
      
maubin
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Could say the User type here instead of object. And now that we've switched to importing from reviewboard.accounts.models.User, is that the value that we should use moving forward in docstrings instead of django.contrib.auth.models.User?

  3. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    Group instead of object.

  4. reviewboard/reviews/builtin_fields.py (Diff revision 1)
     
     
    Show all issues

    User instead of object.

  5. reviewboard/reviews/fields.py (Diff revision 1)
     
     
    Show all issues

    Forgot to add the type here.

  6. reviewboard/reviews/fields.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    And here.

  7. reviewboard/reviews/fields.py (Diff revision 1)
     
     
    Show all issues

    Missing types on some of the attributes here.

  8. 
      
Loading...