Modernize reviewboard.hostingsvcs.base
Review Request #14587 — Created Sept. 4, 2025 and updated
This change modernizes the base classes for hosting services, primarily
adding type annotations and correcting docstrings.The
authorize
andcheck_repository
methods have been made
keyword-only. These were already called by unpacking a dict, and in
practice many of the individual implementations had inconsistent
argument lists which would have made calling with positional arguments
fail. The only places where we were using positional args were in a few
unit tests, which have been updated. I've added
@deprecate_non_keyword_only_args
, but given the existing
inconsistencies, we could probably do this change without a deprecation
phase at all.
Ran unit tests.
Summary | ID |
---|---|
wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs |
Description | From | Last Updated |
---|---|---|
We should add a Raises section to the docs for this. And add a period here. |
![]() |
|
Would you consider making BaseHostingService generic over the client type (e.g., BaseHostingService(Generic[_HostingServiceClientT])) to give the client attribute proper typing? I … |
![]() |
|
Why did this need to become | None? |
![]() |
|
Can we get rid of the comment too? |
![]() |
|
Do we need to document how plan no longer has a default value of None? |
![]() |
|
This sentence got cut off. |
![]() |
|
This needs to be removed. |
![]() |
|
Just randomly thought about this, should we give all of these class variables ClassVar types? |
![]() |
- Change Summary:
-
Pull in a few changes that snuck into a different commit.
- Commits:
-
Summary ID pzyxxllrytpplzmnpllmrsqvktywmqls pzyxxllrytpplzmnpllmrsqvktywmqls - Diff:
-
Revision 2 (+1966 -986)
Checks run (2 succeeded)
- Change Summary:
-
Move to release-7.1.x
- Commits:
-
Summary ID pzyxxllrytpplzmnpllmrsqvktywmqls wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs - Branch:
-
masterrelease-7.1.x
- Diff:
-
Revision 3 (+1978 -998)
Checks run (2 succeeded)

-
-
-
Would you consider making
BaseHostingService
generic over the client type (e.g.,BaseHostingService(Generic[_HostingServiceClientT])
) to give theclient
attribute proper typing?I see in your Forgejo change you set
client: ForgejoClient
which also works, but I'm wondering if the generic route would be better. -
-
-
I don't understand why this was added and, upon reading up about
Never
vsNoReturn
, I still don't really know what it means to have aNever
arg. -
-
-
- Commits:
-
Summary ID wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs - Diff:
-
Revision 4 (+1982 -1008)
Checks run (2 succeeded)
- Commits:
-
Summary ID wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs - Diff:
-
Revision 5 (+2006 -1030)