Summary

Improve typing for LocalSite and LocalSiteManager.

Review Request #14954 — Created March 24, 2026 and submitted April 2, 2026, 11:16 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This adds typing throughout LocalSite and LocalSiteManager, fleshing

out arguments, return types, field relations, and the Local Site

statistics dictionaries.
It also modernizes some of the documentation, which was pretty out of

date, and deprecates some positional arguments.

Testing Done

Unit tests pass.

Commits

Summary	ID
Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	7044a62cf20cdc73e9bef1b587fae3f34d669ee4

Issues

Description	From	Last Updated
Preexisting typo: LocalSit -> LocalSite	david	March 25, 2026, 10:05 p.m.
ValueError is probably more correct.	david	March 25, 2026, 10:05 p.m.
ValueError is probably more correct.	david	March 25, 2026, 10:05 p.m.
This string is missing f-prefixes	david	March 25, 2026, 10:05 p.m.
This docstring doesn't list any exceptions. If we keep the assert it should have AssertionError, if we change to raise_invalid_type …	david	March 25, 2026, 10:06 p.m.
If we want runtime checking that won't be stripped with -O, instead of an assert it might be nice to …	david	March 26, 2026, 11:39 p.m.
SyntaxError: expected ':' Column: 48 Error code: E999	reviewbot	March 25, 2026, 10:23 p.m.
This and below will need to be changed to 8.0 now.	maubin	April 2, 2026, 6:35 a.m.
This and the docstring below can be bumped to RB 10.0 too.	maubin	April 2, 2026, 6:35 a.m.
I know we have reviewboard.accounts.models.User, should we be using that instead of django.contrib.auth.models.User?	maubin	April 2, 2026, 7:33 a.m.
Same comments above.	maubin	April 2, 2026, 6:39 a.m.

flake8 passed.

JSHint passed.

reviewboard/site/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Preexisting typo: LocalSit -> LocalSite
```
reviewboard/site/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
ValueError is probably more correct.
```
reviewboard/site/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
ValueError is probably more correct.
```
reviewboard/site/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This string is missing f-prefixes
```

reviewboard/site/managers.py (Diff revision 1)

The issue has been resolved. Show all issues

This docstring doesn't list any exceptions. If we keep the assert it should have AssertionError, if we change to raise_invalid_type (see next comment) it should be ValueError

reviewboard/site/managers.py (Diff revision 1)

The issue has been dropped. Show all issues

If we want runtime checking that won't be stripped with -O, instead of an assert it might be nice to do:

if isinstance(local_site_or_id, self.model):
    ...
elif isinstance(local_site_or_id, int):
    ...
else:
    raise_invalid_type(...)

chipx86

March 25, 2026, 10:17 p.m.

This is one of those things we shouldn't ever have to come across in production, though I agree it's better safe than sorry.
(FWIW, we don't ship Review Board in optimized mode, but we do ship Power Pack.)
All that said, raise_invalid_type() becomes a mess here, because we're not checking against LocalSite, we're checking against self.model, and it doesn't know the difference. To make it do what it should, we'd need to check against LocalSite, which is technically wrong and logistically annoying, because then the manager has to import the model using the manager.
I want to leave this alone for now, given the constraints here.

chipx86

March 25, 2026, 10:19 p.m.

To be clear on that, if we do:

        if isinstance(local_site_or_id, self.model):
            local_site_id = local_site_or_id.pk
        elif isinstance(local_site_or_id, int):
            local_site_id = local_site_or_id
        else:
            raise_invalid_type(
                local_site_or_id,
                'local_site_or_id must be a LocalSite instance or int',
            )

Then raise_invalid_type() errors at type checking time because we haven't exhausted all available conditions (it says it's not compatible with Never), even though the generic for the manager is typed as a LocalSite.

david

March 26, 2026, 12:58 a.m.
```
ok
```

Change Summary:


Switched some AssertionErrors to ValueErrors.
Fixed a typo in a docstring.
Added missing f-prefixes for a string.

Commits:

	Summary	ID
	Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	4f5733630086b8f6c66bc8bcb7b426bb2d805e3a
	Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	e137b84926d5740056cd0f1f89e1723e54f01103

Diff:

Revision 2 (+458 -146)

Show changes

	reviewboard/site/managers.py
	reviewboard/site/models.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/site/managers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
SyntaxError: expected ':'

Column: 48
Error code: E999
```

Change Summary:

Added a mising colon.

Commits:

	Summary	ID
	Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	e137b84926d5740056cd0f1f89e1723e54f01103
	Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	7c754e38934bf786c3f3b129ca7744a96efaa7fd

Diff:

Revision 3 (+458 -146)

Show changes

	reviewboard/site/managers.py
	reviewboard/site/models.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/site/managers.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This and below will need to be changed to 8.0 now.
```
reviewboard/site/managers.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This and the docstring below can be bumped to RB 10.0 too.
```

reviewboard/site/models.py (Diff revision 3)

The issue has been dropped. Show all issues

I know we have reviewboard.accounts.models.User, should we be using that instead of django.contrib.auth.models.User?

chipx86

April 2, 2026, 6:39 a.m.

It's a bit messy. We'd have to explicitly use django.contrib.auth.models.User for the fields, even though we could avoid it for the types (mostly). Since the Django one is at least a subset of the Review Board one, and we are guaranteed at least that type in most cases, it's probably the right choice right now so we don't have typing errors wherever this is called. But there are places where we probably do want to cast to, say, a RBUser when we're accessing special attributes/methods on the user.

Basically, it's easier and safer to use what we have here for these cases and in this file than to convert.

maubin

April 2, 2026, 7:33 a.m.
```
I see.
```

reviewboard/site/models.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same comments above.
```

Change Summary:

Updated the version and deprecation information for the 8.0 release.

Commits:

	Summary	ID
	Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	7c754e38934bf786c3f3b129ca7744a96efaa7fd
	Improve typing for LocalSite and LocalSiteManager. This adds typing throughout `LocalSite` and `LocalSiteManager`, fleshing out arguments, return types, field relations, and the Local Site statistics dictionaries. It also modernizes some of the documentation, which was pretty out of date, and deprecates some positional arguments.	7044a62cf20cdc73e9bef1b587fae3f34d669ee4

Diff:

Revision 4 (+458 -146)

Show changes

	reviewboard/site/managers.py
	reviewboard/site/models.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (c162da0)