-
Notifications
You must be signed in to change notification settings - Fork 1k
python-ecosys/ujwt: Add ujwt module. #511
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
Conversation
db9add7
to
796f707
Compare
796f707
to
0bbdfc5
Compare
Thanks @miguelgrinberg Code looks good and keen to merge it, I just wanted to bring up two broader points. Just quickly first on the hashlib change though (see also #369 & #435) I think this makes sense for now, the current behaviour is clearly not right. Longer term, I would prefer to see this fixed by adding the missing methods to the built-in implementations.
|
|
The naming issue is annoying but I think important. In this case, as-is, users might think it's a port of the jwt library which is incorrect. Would it be better to have the folder called pyjwt but the python file / installed module left as jwt for consistency with upstream? The For example if someone else wants to release a package that works on both cpython and micropython and uses the jwt library, they need to sprinkler try/except everywhere in imports to check for both names. I'm all in favour of dropping the One minor suggestion... while it's not (yet) common in this repo, a small readme in the folder might be worthwhile even just to link to the upstream module and their docs page? If motivated you could add a note about the limitations of the module, what is/isn't covered - but yeah I haven't done this yet on packages I've worked on so definitely a case of glass houses... |
OK thank you @miguelgrinberg & @andrewleech
I think what we're trying to get towards is the "u" indicates "this is a micropython-specific library and works differently to an existing Python library of the same name". Whereas when possible, we try and provide an actually compatible version of the Python library, even if it's a subset. (i.e. compare old uasyncio which was inspired by but largely incompatible with CPython's vs the "new" v3 version, which we plan to rename to "asyncio").
Yes I was going to suggest exactly this. So we'd have So on micropython you'd do @miguelgrinberg if you're happy with the above, then no need to update the PR, I can take care of it all in the merge (and sort out the manifest.py when I rebase #506). |
@jimmo Yes, I really have no strong opinions on this one way or another. Whatever makes more sense with your current thinking is fine by me. Thanks! |
Added in 9fdf046 with the renaming as discussed plus some minor updates to the test to make it run on CPython with the "real" pyjwt. I didn't include the hashlib change, instead let's solve this properly (see #515). This means that the new pyjwt module won't work until this is merged, but we'll get that done ASAP. Thanks @miguelgrinberg ! |
Fixes #510
Here is my ujwt module. Note that I had to fix a small issue in the hashlib library, which prefers to import the built-in
sha256
class even when micropython-hashlib is installed. Unfortunately the hmac library does not work with the built-in hashlib.Let me know if you need anything else. I have added the packaging files, but I have no idea if there is anything I need to include to push the package to pypi.