jwcrypto: Fix export_to_pem password argument #14037
Conversation
This comment has been minimized.
This comment has been minimized.
stubs/jwcrypto/jwcrypto/jwk.pyi
Outdated
| @@ -201,7 +201,7 @@ class JWK(dict[str, Any]): | |||
| ), | |||
| ) -> None: ... | |||
| def import_from_pem(self, data: bytes, password: bytes | None = None, kid: str | None = None) -> None: ... | |||
| def export_to_pem(self, private_key: bool = False, password: bool = False) -> bytes: ... | |||
| def export_to_pem(self, private_key: bool = False, password: Literal[False] | bytes | None = False) -> bytes: ... | |||
There was a problem hiding this comment.
From what I can see, password is really a required argument, and False is just a sentinel, since writing def export_to_pem(self, private_key=False, password): is illegal syntax. In this case, I think we can do better in the stubs using overloads:
| def export_to_pem(self, private_key: bool = False, password: Literal[False] | bytes | None = False) -> bytes: ... | |
| @overload | |
| def export_to_pem(self, private_key: bool, password: bytes | None) -> bytes: ... | |
| @overload | |
| def export_to_pem(self, private_key: bool = False, *, password: bytes | None) -> bytes: ... |
(This might make stubtest unhappy, im which case we need to add an entry to stubs/jwcrypto/@tests/stubtest_allowlist.txt.)
There was a problem hiding this comment.
My understanding is password is only required if private_key is True. Would this be fine?
@overload
def export_to_pem(self, private_key: Literal[False] = False, password: Literal[False] | bytes | None = False) -> bytes: ...
@overload
def export_to_pem(self, private_key: Literal[True], password: bytes | None) -> bytes: ...There was a problem hiding this comment.
Oops, yes you are right. Your overload looks fine, here are two other alternatives:
from _typeshed import Unused
@overload
def export_to_pem(self, private_key: Literal[False] = False, password: Unused = False) -> bytes: ...
@overload
def export_to_pem(self, private_key: Literal[True], password: bytes | None) -> bytes: ...Unused is a type alias for object. This makes it clear to the reader that password has no effect.
@overload
def export_to_pem(self, private_key: Literal[False] = False) -> bytes: ...
@overload
def export_to_pem(self, private_key: Literal[True], password: bytes | None) -> bytes: ...This prevents errors by the user like export_to_pem(password=b"...") or export_to_pem(True, b"..."), but might be a bit more disruptive, especially when export_to_pem is wrapped by another function with a similar signature.
Please pick which of the three alternatives you think fits best.
There was a problem hiding this comment.
Went with Unused, it feels the most sensible to me.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
No description provided.