Skip to content

jwcrypto: type most of the rest of JWT and JWKSet.generate function #13807

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

Merged
merged 4 commits into from
Apr 12, 2025

Conversation

jkrejcha
Copy link
Contributor

@jkrejcha jkrejcha commented Apr 9, 2025

No description provided.

This comment has been minimized.

Comment on lines 102 to 103
def generate(cls, **kwargs: Any) -> Self: ...
def generate_key(self, **params) -> None: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note that we generally need documentation for each use of Any. Also in this case, at least the required kty argument should be added to the signature. And in theory it's possible to have overloads for the various kty types to includes the extra arguments that some generator functions accept. Something like this:

Suggested change
def generate(cls, **kwargs: Any) -> Self: ...
def generate_key(self, **params) -> None: ...
# `kty` and the other keyword arguments are passed as `params` to the called generator
# function. The possible arguments depend on the value of `kty`.
# TODO: Add overloads for the individual `kty` values.
def generate(cls, *, kty: Literal["oct", "RSA", "EC", "OKP"], **kwargs) -> Self: ...
def generate_key(cls, *, kty: Literal["oct", "RSA", "EC", "OKP"], **kwargs) -> None: ...

@jkrejcha jkrejcha requested a review from srittau April 12, 2025 01:17

This comment has been minimized.

1 similar comment

This comment has been minimized.

@jkrejcha jkrejcha force-pushed the 20250408-jwcrypto-fix-export branch 2 times, most recently from 5544378 to d4df742 Compare April 12, 2025 04:42

This comment has been minimized.

srittau
srittau previously approved these changes Apr 12, 2025
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks!

@srittau srittau dismissed their stale review April 12, 2025 13:02

stubtest fails

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks, nearly good to go, except that JWKKeyTypeSupported, JWKOperationSupported, and JWKUseSupported need to be prefixed with an underscore, to mark that they are not available at runtime. This should also fix the CI failure.

@jkrejcha jkrejcha force-pushed the 20250408-jwcrypto-fix-export branch from d4df742 to 544f489 Compare April 12, 2025 16:44
@jkrejcha
Copy link
Contributor Author

jkrejcha commented Apr 12, 2025

Thanks, nearly good to go, except that JWKKeyTypeSupported, JWKOperationSupported, and JWKUseSupported need to be prefixed with an underscore, to mark that they are not available at runtime. This should also fix the CI failure.

Oop, ya my bad, fixed up

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit 648ed7b into python:main Apr 12, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants