-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid surprising mutation and remove Any from the kwargs type. #6
Conversation
Use the types-requests stubs to annotate the kwargs. Ideally we don't import _Names from a stub namespace but let the type checker infer the ParamSpec.kwargs from its use. But ParamSpec can't be used as where a Type is expected.
Was already like this for black. Running tox locally with a local venv in the project directory was failing (and taking unnecessarily long)... Actually still does: flake8 + isort + black > uv 😉
setattr(self, k, v) | ||
else: | ||
# store the remainder so we can inject it in the ``request`` method. | ||
self._request_kwargs[k] = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose not to annotate self._request_kwargs
as RequestKwargs
, because mypy only allows assignment in it with literals. The fact that k
came from the same type...
PyRight either is smarter or doesn't seem to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with switching to pyright in CI instead of mypy if that makes life better
another_one = APIClient.configure_from(MyConfig) | ||
|
||
assert one.auth == ("djkhaled", "youplayedyourself") | ||
assert another_one.auth == ("djkhaled", "youplayedyourself") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading #3... Does this actually work? Or is this one of the surprises Victorien meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a different thing. A surprising one indeed 😬
There is no gitmoji for happy little accident. But this