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
There are no open issues
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)
Loading...