• 
      

    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)