Modernize reviewboard.hostingsvcs.base

Review Request #14587 — Created Sept. 4, 2025 and updated

Information

Review Board
release-7.1.x

Reviewers

This change modernizes the base classes for hosting services, primarily
adding type annotations and correcting docstrings.

The authorize and check_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
Modernize reviewboard.hostingsvcs.base
This change modernizes the base classes for hosting services, primarily adding type annotations and correcting docstrings. The `authorize` and `check_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. Testing Done: Ran unit tests.
wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs
Description From Last Updated

We should add a Raises section to the docs for this. And add a period here.

maubinmaubin

Would you consider making BaseHostingService generic over the client type (e.g., BaseHostingService(Generic[_HostingServiceClientT])) to give the client attribute proper typing? I …

maubinmaubin

Why did this need to become | None?

maubinmaubin

Can we get rid of the comment too?

maubinmaubin

Do we need to document how plan no longer has a default value of None?

maubinmaubin

This sentence got cut off.

maubinmaubin

This needs to be removed.

maubinmaubin

Just randomly thought about this, should we give all of these class variables ClassVar types?

maubinmaubin
david
david
maubin
  1. Great cleanup thank you.

  2. reviewboard/hostingsvcs/assembla.py (Diff revision 3)
     
     
    Show all issues

    We should add a Raises section to the docs for this. And add a period here.

  3. Show all issues

    Would you consider making BaseHostingService generic over the client type (e.g., BaseHostingService(Generic[_HostingServiceClientT])) to give the client 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.

    1. I've tried repeatedly to do that and it's turning out to be very hard. I'll keep poking at it (for a separate change) but I'm not sure I'll get there.

  4. Show all issues

    Why did this need to become | None?

    1. Not all hosting services have a path field (for example, gerrit operates on only gerrit_url and gerrit_project_name)

  5. reviewboard/hostingsvcs/base/hosting_service.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    Can we get rid of the comment too?

  6. reviewboard/hostingsvcs/base/http.py (Diff revision 3)
     
     

    I don't understand why this was added and, upon reading up about Never vs NoReturn, I still don't really know what it means to have a Never arg.

    1. Never is used for handling theoretically unreachable situations based on types.

      For example:

      def fn(a: str | int | None):
          if isinstance(a, str):
              ...
          elif isinstance(a, int):
              ...
          elif a is None:
              ...
          else:
              ... # this code gets flagged by type checkers as unreachable because it's theoretically impossible from the types.
      

      In this case, to avoid the type checker issuing a warning, we can do:

      def fn(a: str | int | None):
          if isinstance(a, str):
              ...
          elif isinstance(a, int):
              ...
          elif a is None:
              ...
          else:
              _log_and_raise(a, ...)
      

      https://typing.python.org/en/latest/guides/unreachable.html has more detail.

  7. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues

    Do we need to document how plan no longer has a default value of None?

    1. I don't think so? It was never correct that plan could have been None.

  8. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     
    Show all issues

    This sentence got cut off.

  9. reviewboard/hostingsvcs/rbgateway.py (Diff revision 3)
     
     
     
    Show all issues

    This needs to be removed.

  10. 
      
david
maubin
  1. Ship It!
  2. 
      
maubin
  1. 
      
  2. Show all issues

    Just randomly thought about this, should we give all of these class variables ClassVar types?

    1. Yes, we probably should.

  3. 
      
david
Review request changed
Commits:
Summary ID
Modernize reviewboard.hostingsvcs.base
This change modernizes the base classes for hosting services, primarily adding type annotations and correcting docstrings. The `authorize` and `check_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. Testing Done: Ran unit tests.
wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs
Modernize reviewboard.hostingsvcs.base
This change modernizes the base classes for hosting services, primarily adding type annotations and correcting docstrings. The `authorize` and `check_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. Testing Done: Ran unit tests.
wmlkkkqwtzqrmyypyuwxwxyxrsspsrvs

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
maubin
  1. Ship It!
  2.