-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(types): update type hints in web3._utils.method_formatters.py #3669
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
Conversation
web3/_utils/method_formatters.py
Outdated
|
|
||
|
|
||
| def get_error_formatters( | ||
| method_name: Union[RPCEndpoint, Callable[..., RPCEndpoint]] |
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.
Is method_name actually supposed to be a Union[RPCEndpoint, Callable[..., RPCEndpoint]]?
I notice Union[RPCEndpoint, Callable[..., RPCEndpoint]] is used throughout the codebase but the code itself seems to assume method_name is always a string value, never callable. Should this be updated as well?
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.
Oh, nice catch. Yeah, these can only be RPCEndpoints once they're here.
06680f0 to
6ec8bed
Compare
6ec8bed to
d8f7ccf
Compare
|
|
||
| @to_tuple | ||
| def apply_module_to_formatters( | ||
| formatters: Tuple[Callable[..., TReturn]], |
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.
Curious about the reasoning behind this change?
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.
This change just indicates to mypy that the function will work with any iterable of callables.
The original hint indicates that the function must take a tuple input, so while it didn't cause any type errors internally here it can cause unneeded type errors for downstream users.
web3/_utils/method_formatters.py
Outdated
|
|
||
|
|
||
| def get_error_formatters( | ||
| method_name: Union[RPCEndpoint, Callable[..., RPCEndpoint]] |
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.
Oh, nice catch. Yeah, these can only be RPCEndpoints once they're here.
1e81a1f to
ce79a61
Compare
ce79a61 to
5815be3
Compare
kclowes
left a comment
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.
This looks good to me, thank you! Will add a newsfragment and merge!
| def method_selector_fn( | ||
| self, | ||
| ) -> Callable[..., Union[RPCEndpoint, Callable[..., RPCEndpoint]]]: | ||
| ) -> Callable[..., RPCEndpoint]: |
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.
👌🏼
What was wrong?
get_result_formattershad return typeDict[str, Callable[..., Any]]which is incorrect because the function returns a callabletoolz.composeobjectget_error_formatters, andget_null_result_formattersboth had return typeCallable[..., Any]which is techincally correct but could be made more specific for clarity for contributors.Related to Issue #N/A
Closes #N/A
How was it fixed?
I've changed the return type for all 3 functions to
Callable[[RPCResponse], Any].Todo:
I did not add an entry to release notes as this change is not relevant for users.
Cute Animal Picture