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

RFC: Add type hints #344

Merged
merged 4 commits into from
Apr 8, 2018
Merged

RFC: Add type hints #344

merged 4 commits into from
Apr 8, 2018

Conversation

jacopofar
Copy link
Contributor

I decided to use the time my company, Flixbus, dedicates to FOSS projects to contribute to this package.

This PR adds type hints as described in issue #312 and includes mypy in Travis configuration. This means that IDEs can automatically detect type errors and results from static check tools can improve.

There are a few doubts about which I'd like a feedback and for which I'm ready to work to change/fix:

  • Mypy complains about PyJWT.encode not being a subtype of PyJWS.encode, and the same for decode. In the case of decode the problem is that one calls the token jwt and the other jws, so I just called both token. As for encode the problem is that JWS expects bytes and JWT a dictionary, so I changed JWS.encode to JWS.encode_bytes. In the documentation there are no examples about using that function, but it is a breaking change for who uses it.

  • The change required to import the typing module in a try...catch, so it can be imported when checking the types but is ignored during normal usage of the library. It's a bit ugly but I couldn't find other ways to have it work on Python 2.7

  • I didn't update the changelog or bump the version, if JWS.encode is considered part of the API I guess the major version number is affected

@coveralls
Copy link

coveralls commented Apr 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling af795a8 on flix-tech:add-type-hints into ee2ab9f on jpadilla:master.

@jacopofar jacopofar mentioned this pull request Apr 6, 2018
jwt/api_jws.py Outdated
def encode(self, payload, key, algorithm='HS256', headers=None,
json_encoder=None):
def encode_bytes(self,
payload, # type: bytes
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering if we could leave this as .encode() and make the type for payload Union[bytes, Dict]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible, doing so mypy says:
jwt/api_jwt.py:57: error: Unsupported target for indexed assignment
because the function is using the payload it as a dictionary. But before that there's a check for it and an error being thrown if it's not so I think it's fine to silence it.

The new commit does that and revert encode_bytes to encode

Copy link
Owner

@jpadilla jpadilla left a comment

Choose a reason for hiding this comment

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

I think this is definitely a great first step, thank you so much!

@jpadilla jpadilla merged commit aed7305 into jpadilla:master Apr 8, 2018
@jpadilla jpadilla added this to the v1.6.2 milestone May 19, 2018
@@ -55,7 +66,12 @@ def encode(self, payload, key, algorithm='HS256', headers=None,
json_payload, key, algorithm, headers, json_encoder
)

def decode(self, jwt, key='', verify=True, algorithms=None, options=None,

Choose a reason for hiding this comment

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

change jwt -> token, changes public interface and broken my code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't see it as a major change as the argument is positional, turns out I was wrong and there are users with the same problem. It has been reverted here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants