Improve RegisteredResponse init doc & signature
#1126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`RegisteredResponse.__init__` allowed any keyword arguments via
`**kwargs` usage, but would later pass these to `responses.Response`
via the `add()` or `replace()` methods, at which point there would be
a `TypeError`.
This can make it hard to understand what features are being exposed
during development. e.g., "Was the parameter name `status` or
`status_code`?"
To resolve and provide an improved experience for development with
`RegisteredResponse`, the init parameters are better documented and
the signature is improved to explicitly enumerate supported args.
1. Add `:param ...:` docstring items
2. Explicitly enumerate parameters, removing `**kwargs: Any`
3. Document which parameters come from `responses.Response`, which
come from `responses.BaseResponse`, and which ones are omitted (and
why)
4. Update `service` and `method` to use Literals to show the values
In terms of lost functionality, technically the following three
parameters are removed in this change:
- auto_calculate_content_length
- passthrough
- match_querystring
However, given that `_testing` has a weaker backwards compatibility
contract than the rest of the SDK and there are no known usages for
these, this seems like a good change rather than a bad one. We can
always add `auto_calculate_content_length` in the future. The other
two would require stronger justification, since
- match_querystring is deprecated in `responses`
- passthrough doesn't seem applicable to any normal uses of
`globus_sdk._testing`
- we don't forbid anyone from using `responses` directly
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus,
kurtmckee,
m1yag1 and
MaxTueckeGlobus
as code owners
kurtmckee
reviewed
Jan 13, 2025
| service: str | None = None, | ||
| method: str = responses.GET, | ||
| service: ( | ||
| Literal[ |
There was a problem hiding this comment.
Looks like we have a pyupgrade configuration issue: above, Literal is imported conditionally for Python 3.7 and below, which we no longer support.
This should become t.Literal in a subsequent PR.
Co-authored-by: Kurt McKee <[email protected]>
Compare in a test against the options supported by `http.HTTPMethod`, and thereby compare against the strings supported by `responses`. Add `OPTIONS`, `CONNECT`, and `TRACE` for completeness.
In order to ensure that we aren't overriding a potentially new default in `responses` by passing `()` explicitly to it, simply default to `None` and only pass a value if it is non-null. Also refine docstrings.
kurtmckee
approved these changes
Jan 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RegisteredResponse.__init__allowed any keyword arguments via**kwargsusage, but would later pass these toresponses.Responsevia theadd()orreplace()methods, at which point there could be aTypeError.This can make it hard to understand what features are being exposed during development. e.g., "Was the parameter name
statusorstatus_code?"To resolve and provide an improved experience for development with
RegisteredResponse, the init parameters are better documented and the signature is improved to explicitly enumerate supported args.:param ...:docstring items**kwargs: Anyresponses.Response, which come fromresponses.BaseResponse, and which ones are omitted (and why)serviceandmethodto use Literals to show the valuesIn terms of lost functionality, technically the following three parameters are removed in this change:
However, given that
_testinghas a weaker backwards compatibility contract than the rest of the SDK and there are no known usages for these, this seems like a good change rather than a bad one. We can always addauto_calculate_content_lengthin the future. The other two would require stronger justification, sinceI was motivated to tackle this while playing with some editor support for code completion and docstring inspection, and I found that
RegisteredResponsedidn't have the information I wanted.In particular, I wanted to make sure I was using correct parameter names, and the loaded class defintion and docstring didn't provide this information.
📚 Documentation preview 📚: https://globus-sdk-python--1126.org.readthedocs.build/en/1126/