• 
      

    Modernize base resource classes.

    Review Request #14256 — Created Dec. 4, 2024 and submitted

    Information

    RBTools
    master

    Reviewers

    This change adds/updates typing and documentation for the base resource
    classes.

    Ran unit tests.

    Summary ID
    Modernize base resource classes.
    This change adds/updates typing and documentation for the base resource classes. Testing Done: Ran unit tests.
    d6b3813fa92b69b4e084901c1d25a5114a237cc5
    Description From Last Updated

    'typing.Type' imported but unused Column: 1 Error code: F401

    reviewbotreviewbot

    line too long (80 > 79 characters) Column: 80 Error code: E501

    reviewbotreviewbot

    While you're here could you fix up this sentence.

    maubinmaubin

    In RB6+ we have QueryArgs and HTTPHeader type aliases defined in /reviewboard/hostingsvcs/base/http.py. Would it be appropriate to use them here?

    maubinmaubin

    Small nit which I'm not sure even matters but I notice we usually use * instead of - in docstrings.

    maubinmaubin

    Could we use JSONDict instead?

    maubinmaubin

    We can avoid the issubclass and optimize out the call if we do: assert token is None or not issubclass(...)

    chipx86chipx86

    If we go with Python 3.9+, we could do key = name.removeprefix(_EXTRA_DATA_PREFIX)

    chipx86chipx86

    Can you make these parameter names inline literals?

    chipx86chipx86

    This reads kind of awkwardly, given the length of the value. Maybe alias it, or do: SPECIAL_LINKS: dict[ str, tuple[...] …

    chipx86chipx86

    Missing Returns.

    chipx86chipx86

    No blank line here.

    chipx86chipx86

    Missing docs with Version Added. Also, probably should be Optional[Union[...]]. I know it's the same thing, but Optional[] is more …

    chipx86chipx86

    Let's swap the order here, keep it alphabetical.

    chipx86chipx86

    Are these Any or JSONValue? Maybe the former is safer all around, but floating the question.

    chipx86chipx86

    Can be later, but it'd be nice to start moving to keyword-only arguments so these are all easier to manage …

    chipx86chipx86

    We're doing this | and attribute access per-item in data. Let's build the exclusion list only once before the loop.

    chipx86chipx86
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    david
    maubin
    1. 
        
    2. rbtools/api/request.py (Diff revision 2)
       
       
      Show all issues

      While you're here could you fix up this sentence.

    3. rbtools/api/request.py (Diff revision 2)
       
       
       
      Show all issues

      In RB6+ we have QueryArgs and HTTPHeader type aliases defined in /reviewboard/hostingsvcs/base/http.py. Would it be appropriate to use them here?

      1. If we used these in more places I'd say yes, but I don't think it's important given the small number of occurrences.

    4. rbtools/api/resource/base.py (Diff revision 2)
       
       
       
      Show all issues

      Small nit which I'm not sure even matters but I notice we usually use * instead of - in docstrings.

    5. rbtools/api/resource/base.py (Diff revision 2)
       
       
      Show all issues

      Could we use JSONDict instead?

      1. I don't think JSONDict is entirely accurate here (we don't submit POST requests as JSON, though we probably should allow that). I'm still trying to figure out the best way to do typing for create and update methods generally, so I'd rather keep this as-is for now.

    6. 
        
    david
    maubin
    1. Ship It!
    2. 
        
    chipx86
    1. Looks very good. Just a few comments here.

      One thing I was trying to think about when looking at this is whether we should be typing resource payloads as list[...]/dict[...] or Sequence[...]/Mapping[...]. I don't think we want to allow mutability for the caller, so the latter is probably more correct, but then I don't know whether that imposes new challenges when populating the resource wrapper.

    2. rbtools/api/factory.py (Diff revision 3)
       
       
       
      Show all issues

      We can avoid the issubclass and optimize out the call if we do:

      assert token is None or not issubclass(...)
      
      1. I feel like that's a lot more confusing, and this isn't a hot codepath.

    3. rbtools/api/resource/base.py (Diff revision 3)
       
       
      Show all issues

      If we go with Python 3.9+, we could do key = name.removeprefix(_EXTRA_DATA_PREFIX)

    4. rbtools/api/resource/base.py (Diff revision 3)
       
       
       
      Show all issues

      Can you make these parameter names inline literals?

    5. rbtools/api/resource/base.py (Diff revision 3)
       
       
       
      Show all issues

      This reads kind of awkwardly, given the length of the value. Maybe alias it, or do:

      SPECIAL_LINKS: dict[
          str,
          tuple[...]
      ] = {
          ...
      }
      

      Also, we should use Mapping, since this isn't meant to be mutated.

    6. rbtools/api/resource/base.py (Diff revision 3)
       
       
       
       
       
      Show all issues

      Missing Returns.

    7. rbtools/api/resource/base.py (Diff revision 3)
       
       
       
       
      Show all issues

      No blank line here.

    8. rbtools/api/resource/base.py (Diff revision 3)
       
       
      Show all issues

      Missing docs with Version Added.

      Also, probably should be Optional[Union[...]]. I know it's the same thing, but Optional[] is more consistent.

      1. I feel like in this case that's more confusing, and Optional is deprecated, so we'll eventually be moving away from Optional[X] to X | None. I guess given __future__.annotations we could do that today if we wanted.

    9. rbtools/api/resource/base.py (Diff revision 3)
       
       
      Show all issues

      Let's swap the order here, keep it alphabetical.

    10. rbtools/api/resource/base.py (Diff revision 3)
       
       
      Show all issues

      Are these Any or JSONValue? Maybe the former is safer all around, but floating the question.

      1. This is something I parameterize in a later change.

    11. rbtools/api/resource/base.py (Diff revision 3)
       
       
       
       
       
      Show all issues

      Can be later, but it'd be nice to start moving to keyword-only arguments so these are all easier to manage over time.

      Same applies to other base resource functions.

    12. rbtools/api/resource/base.py (Diff revision 3)
       
       
       
      Show all issues

      We're doing this | and attribute access per-item in data. Let's build the exclusion list only once before the loop.

    13. 
        
    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (67312a2)