Split PyJWT/PyJWS classes to tighten type interfaces #559
Conversation
| **kwargs, | ||
| ): | ||
| ) -> Dict[str, Any]: |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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