Modernize reviewboard.hostingsvcs.base

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

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

These should be in the same import group.

chipx86chipx86

The previous form was a bit easier to read.

chipx86chipx86

Might be nice to make these kwonly as well.

chipx86chipx86
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.

    2. I went down the rabbit hole on this. Meanings changed a bit over time (NoReturn involved exceptions, Never meant "can never return"), but it seems this has changed.

      The latest spec says this:

      Since Python 3.11, the typing module contains a special form Never. It represents the bottom type, a type that represents the empty set of Python objects.

      The Never type is equivalent to NoReturn, which is discussed above. The NoReturn type is conventionally used in return annotations of functions, and Never is typically used in other locations, but the two types are completely interchangeable.

      The "Unreachable Code and Exhaustiveness Checking" guide is showing Never to be used for code that can never be reached, wiring off that code path from type checkers. But that's not what this function is doing.

      Given that, it sounds like NoReturn is in fact the more correct option, even though technically they're equivalent. This function is logging something and raising, but is a valid code path that can be called in valid circumstances and would remain checked. It's not a code path that can't be hit, but rather a validation check for implementations that might be incorrect.

  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
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/base/hosting_service.py (Diff revision 5)
     
     
     
     
    Show all issues

    These should be in the same import group.

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

    The previous form was a bit easier to read.

  4. reviewboard/hostingsvcs/base/paginator.py (Diff revision 5)
     
     
     
     
    Show all issues

    Might be nice to make these kwonly as well.

  5. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.1.x (7fad37f)