Summary

Improve typing for LocalSite and LocalSiteManager.

Review Request #14954 — Created March 23, 2026 and updated March 25, 2026, 4:58 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.	7c754e38934bf786c3f3b129ca7744a96efaa7fd

Issues

Description	From	Last Updated
Preexisting typo: LocalSit -> LocalSite	david	March 25, 2026, 2:05 p.m.
ValueError is probably more correct.	david	March 25, 2026, 2:05 p.m.
ValueError is probably more correct.	david	March 25, 2026, 2:05 p.m.
This string is missing f-prefixes	david	March 25, 2026, 2: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, 2: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, 3:39 p.m.
SyntaxError: expected ':' Column: 48 Error code: E999	reviewbot	March 25, 2026, 2:23 p.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, 2: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, 2: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 25, 2026, 4:58 p.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!
```