Skip to content
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

Use TypedDict for **kwargs type checking #2849

Merged
merged 3 commits into from
May 3, 2024

Conversation

e3243eric
Copy link
Contributor

@e3243eric e3243eric commented Feb 25, 2023

What was wrong?

web3.py/web3/eth/eth.py

Lines 551 to 555 in 168fcea

# todo: Update Any to stricter kwarg checking with TxParams
# https://github.com/python/mypy/issues/4441
def modify_transaction(
self, transaction_hash: _Hash32, **transaction_params: Any
) -> HexBytes:

After Mypy 0.981, it supports precise type annotations for **kwargs using TypedDict (python/mypy#13471), like **transaction_params: Any can be rewritten as **transaction_params: Unpack[TxParams].

How was it fixed?

  • Upgrade Mypy to 0.981.
  • Update **kwarg type checking with Unpack[TxParams].
  • Add Unpack type. The Unpack is a built-in type after Python 3.11, so I updated the web3/_utils/compat.

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@e3243eric e3243eric changed the title **kwargs type checking with TypedDict Use TypedDict for **kwargs type checking Feb 25, 2023
@e3243eric
Copy link
Contributor Author

e3243eric commented Mar 1, 2023

The remaining lint errors are from the submodule ethpm-spec.

@pacrob
Copy link
Contributor

pacrob commented Sep 5, 2023

Unpack does look useful, but we can't include something that is labeled "experimental" and "incomplete" in a release. Thanks for bringing it up, though. I'll leave this open and check back on it in a few months.

@fselmo
Copy link
Collaborator

fselmo commented Apr 17, 2024

As far as I know this is still an experimental feature. I'm open to including it, but I'm not certain that it's desirable at this stage. Considering this for the next major release, v8, might be best.

@fselmo fselmo added the v8 breaking changes for v8 label Apr 17, 2024
@pacrob pacrob force-pushed the kwargs-checking branch from 91e530e to 46360ad Compare May 3, 2024 18:38
@pacrob
Copy link
Contributor

pacrob commented May 3, 2024

Unpack is out of experimental in more recent mypy versions. I've rebased and applied the changes to the async versions. Can discuss inclusion timeline.

@fselmo
Copy link
Collaborator

fselmo commented May 3, 2024

Nice find. I think we should aim to put in v7 then. Thoughts?

@pacrob pacrob force-pushed the kwargs-checking branch from 0dd913f to 20dfab8 Compare May 3, 2024 20:35
@pacrob pacrob force-pushed the kwargs-checking branch from 20dfab8 to 18ad64d Compare May 3, 2024 20:44
@pacrob
Copy link
Contributor

pacrob commented May 3, 2024

No reason not to include in v7 from me.

@fselmo fselmo added v7 breaking changes considered for v7 and removed v8 breaking changes for v8 labels May 3, 2024
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@pacrob pacrob merged commit 3954318 into ethereum:main May 3, 2024
71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v7 breaking changes considered for v7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants