Modernize base resource classes.
Review Request #14256 — Created Dec. 4, 2024 and submitted
This change adds/updates typing and documentation for the base resource
classes.
Ran unit tests.
Summary | ID |
---|---|
d6b3813fa92b69b4e084901c1d25a5114a237cc5 |
Description | From | Last Updated | ||
---|---|---|---|---|
'typing.Type' imported but unused Column: 1 Error code: F401 |
![]() |
|||
line too long (80 > 79 characters) Column: 80 Error code: E501 |
![]() |
|||
While you're here could you fix up this sentence. |
![]() |
|||
In RB6+ we have QueryArgs and HTTPHeader type aliases defined in /reviewboard/hostingsvcs/base/http.py. Would it be appropriate to use them here? |
![]() |
|||
Small nit which I'm not sure even matters but I notice we usually use * instead of - in docstrings. |
![]() |
|||
Could we use JSONDict instead? |
![]() |
|||
We can avoid the issubclass and optimize out the call if we do: assert token is None or not issubclass(...) |
|
|||
If we go with Python 3.9+, we could do key = name.removeprefix(_EXTRA_DATA_PREFIX) |
|
|||
Can you make these parameter names inline literals? |
|
|||
This reads kind of awkwardly, given the length of the value. Maybe alias it, or do: SPECIAL_LINKS: dict[ str, tuple[...] … |
|
|||
Missing Returns. |
|
|||
No blank line here. |
|
|||
Missing docs with Version Added. Also, probably should be Optional[Union[...]]. I know it's the same thing, but Optional[] is more … |
|
|||
Let's swap the order here, keep it alphabetical. |
|
|||
Are these Any or JSONValue? Maybe the former is safer all around, but floating the question. |
|
|||
Can be later, but it'd be nice to start moving to keyword-only arguments so these are all easier to manage … |
|
|||
We're doing this | and attribute access per-item in data. Let's build the exclusion list only once before the loop. |
|
|||
There are no open issues |
- Commits:
-
Summary ID b7a94b38bba88ba32623c924b596a6ded24ad8c9 4f5819e99c65406fd5eb7130f07e31f08fd55875
Checks run (2 succeeded)
- Commits:
-
Summary ID 4f5819e99c65406fd5eb7130f07e31f08fd55875 d5e0de45c634baab7c4e56c0c30207ac09edd4c5
Checks run (2 succeeded)
-
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[...]
orSequence[...]
/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. -
We can avoid the
issubclass
and optimize out the call if we do:assert token is None or not issubclass(...)
-
-
-
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. -
-
-
Missing docs with
Version Added
.Also, probably should be
Optional[Union[...]]
. I know it's the same thing, butOptional[]
is more consistent. -
-
-
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.
-
We're doing this
|
and attribute access per-item indata
. Let's build the exclusion list only once before the loop.