-
-
Notifications
You must be signed in to change notification settings - Fork 676
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
Split PyJWT/PyJWS classes to tighten type interfaces #559
Conversation
**kwargs, | ||
): | ||
) -> Dict[str, Any]: |
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 is a perfectly fine solution, but if you wanted to maintain backwards-compatibility, this could remain the same, but use @overload
.
It would look something like:
@overload
def decode(self, jwt: str, key: str = ..., algorithms: Optional[List[str]] = ..., options: Optional[Dict] = ..., complete: Literal[True], **kwargs: Any) -> Dict[str, Any]: ...
@overload
def decode(self, jwt: str, key: str = ..., algorithms: Optional[List[str]] = ..., options: Optional[Dict] = ..., complete: Literal[False] = ..., **kwargs: Any) -> str: ...
def decode(self, jwt: str, key: str = "", algorithms: Optional[List[str]] = None, options: Optional[Dict] = None, complete: bool = False, **kwargs) -> Union[str, Dict[str, Any]]:
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.
We're prepping for version 2.0, so this is the chance to break backward compatibility if we must. IMO, we should aim for the desired interface rather than force support for something we can drop.
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.
I played around with the overload
idea a bit. Unfortunately, Literal
was only introduced in Python 3.8.
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.
Yes, that's right. Not sure you can do the overload without it.
If you wanted to go that route, you could import from typing_extensions
for backwards compatibility (adding that to the dependencies). Alternatively, only do the overload in Python 3.8+ (if sys.version_info >= (3, 8)
, and users of older versions will have to put up with the Union
(though I suspect most Mypy users are already using 3.8+).
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.
Your current proposal is probably the easier option though.
@@ -215,6 +215,27 @@ def test_decodes_valid_jws(self, jws, payload): | |||
|
|||
assert decoded_payload == payload | |||
|
|||
def test_decodes_complete_valid_jws(self, jws, payload): |
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.
If you start enforcing Mypy on CI, I'd suggest adding some annotations to the tests (and adding tests/ to the Mypy run), as they can help catch API mistakes, as this is the only place in the code that actually uses much of the library. We've done this recently on aiohttp-jinja2, where some annotations were incorrect and only getting noticed by users previously.
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.
Certainly would be useful, but it seems a much bigger issue to tackle and so I think is outside the scope for this particular PR. Help there would be welcome.
P.S. I'm not the maintainer of this project, just a contributor.
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.
Of course, just giving some ideas for the future.
If you want to tackle it, I'd start by adding a .mypy.ini
file, for example:
https://github.com/aio-libs/aiohttp-jinja2/blob/master/.mypy.ini
That may be a little too strict for this project, so play around with the options (but, you'll want to change disallow_untyped_defs
to True
under tests, we skip it because we don't have any important things in the parameters).
Then, adding CI support can be done with something as simple as:
https://github.com/mlowijs/tesla_api/blob/typing/.github/workflows/ci.yaml#L16-L23
Let me know if you need any other help.
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 is great, thanks @jdufresne @Dreamsorcerer
The class PyJWT was previously a subclass of PyJWS. However, this combination does not follow the Liskov substitution principle. That is, using PyJWT in place of a PyJWS would not produce correct results or follow type contracts. While these classes look to share a common interface it doesn't go beyond the method names "encode" and "decode" and so is merely superficial. The classes have been split into two. PyJWT now uses composition instead of inheritance to achieve the desired behavior. Splitting the classes in this way allowed for precising the type interfaces. The complete parameter to .decode() has been removed. This argument was used to alter the return type of .decode(). Now, there are two different methods with more explicit return types and values. The new method name is .decode_complete(). This fills the previous role filled by .decode(..., complete=True). Closes #554, #396, #394 Co-authored-by: Sam Bull <git@sambull.org>
The class PyJWT was previously a subclass of PyJWS. However, this
combination does not follow the Liskov substitution principle. That is,
using PyJWT in place of a PyJWS would not produce correct results or
follow type contracts.
While these classes look to share a common interface it doesn't go
beyond the method names "encode" and "decode" and so is merely
superficial.
The classes have been split into two. PyJWT now uses composition instead
of inheritance to achieve the desired behavior. Splitting the classes in
this way allowed for precising the type interfaces.
The complete parameter to .decode() has been removed. This argument was
used to alter the return type of .decode(). Now, there are two different
methods with more explicit return types and values. The new method name
is .decode_complete(). This fills the previous role filled by
.decode(..., complete=True).
Closes #554, #396, #394
Co-authored-by: Sam Bull git@sambull.org