Repository navigation
Stricter typing for EventHooks - #2266
Merged
Merged
Conversation
relsunkaev
force-pushed
the
master
branch
2 times, most recently
from
June 15, 2022 23:19
d09364f to
1883df4
Compare
Member
|
Hiya - thanks for getting involved! From my perspective there's too many different sets of changes in this pull request at one time. For example, I'd want to work through event hooks as one isolated PR. extensions as one isolated PR. etc. Some of these we might want, some we might not, and and some we might want to adapt. |
Contributor
Author
|
Sounds good! I'll split up the PR and try to be more granular. I can see I got a little carried away there. |
relsunkaev
changed the title
Contributor
Author
|
Updated PR to narrow the scope. |
Contributor
|
This is a bit surprising to me, as I don't see there's any semantic difference between e.g. Is pyright's "need explicit Any" okay to comply with here? We've got similar "typing strictness" questions in the past where users use strict settings and tools then expect all libraries to comply. Ideally that would only be configured on user code. |
Contributor
Author
|
The way I understand it Although having to add On the note of applying strict to library code, users turn on strict to achieve a certain level of type safety and should be made aware if they're using a library that doesn't align with those goals. If anything, when using strict, one's dependencies should probably be more type-safe than their own code as the effort required to fix any errors/unexpected behaviors is much larger there. There is also an added benefit to users that don't care about type safety is ergonomics (autocomplete, method parameter hints, etc.). |
Member
|
Okay, so our baseline for typing changes like this should be… does this improve the tooling situation for our users. I’ve not been able to install
Ideally we’d have any checks here integrated into our tooling. I don’t know if there are any |
Contributor
I recommend using
Yep agreed. And I think "does our public API pass Pyright in strict mode" is the bar to meet (not that we have to fully type anything, we can always use
ResultsType completeness score: 26.9%
This is a lot more manual. I've been including them in my PRs but it'd be very hard to take a screenshot of every issue.
In my experience running mypy in strict mode is "close enough". The trick is we'd have to have mypy in strict mode pass on our tests to reproduce the "user experience". I think this should ultimately be the end goal, but we'll probably have to start by cleaning up typing without CI enforcement, then enforce mypy strict mode on all/some of the
I have never had luck with this; I think mypy in strict mode will do |
Contributor
|
As for this PR: I'm guessing it was larger before but now it's pretty well self contained, I'm inclined to move forward with it if you don't have any objections @tomchristie |
adriangb
approved these changes
Oct 12, 2022
| max_redirects: int = DEFAULT_MAX_REDIRECTS, | ||
| event_hooks: typing.Optional[ | ||
| typing.Mapping[str, typing.List[typing.Callable]] | ||
| typing.Mapping[str, typing.List[typing.Callable[..., typing.Any]]] |
Contributor
There was a problem hiding this comment.
Suggested change
| typing.Mapping[str, typing.List[typing.Callable[..., typing.Any]]] | |
| typing.Mapping[str, typing.List[EventHook]] |
Contributor
There was a problem hiding this comment.
I’ll make a followup PR to fix this
lovelydinosaur
changed the title
Member
|
Thanks @relsunkaev! |
samclearman
pushed a commit
to titanmsp/httpx
that referenced
this pull request
Apr 26, 2025
Co-authored-by: Tom Christie <tom@tomchristie.com> Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
This file contains hidden or 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
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.
The public API is now "fully typed" with generous use of
Any. I have ideas of how to get rid of those but wanted to limit the scope of this PR.Notable Changes
[..., typing.Any]to all Callables in_client.pypending narrower typing.RequestDatatype alias asDict[Any, Any].