Skip to content

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

Closed

Conversation

miguelgrinberg
Copy link
Contributor

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.

@miguelgrinberg miguelgrinberg force-pushed the ujwt-module branch 2 times, most recently from db9add7 to 796f707 Compare July 26, 2022 23:10
@jimmo
Copy link
Member

jimmo commented Aug 5, 2022

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.

  1. Should it be called "ujwt"?
    a) It matches the implementation of PyJWT, so should it be "upyjwt" ? I know that when you install PyJWT, you get the "jwt" module though. Frustratingly the "jwt" library on PyPi is a different thing. (It's pyserial all over again...)
    b) Should we be using the "u" prefix? I know "urequests" is that way, but that's kind of a historical accident. The way I want this to work is MicroPython users should just be able to write Python code and use the module names they're familiar with, and reserve the "u" prefix for modules that are micropython-specific and different to anything you'd see in CPython. I've been pushing on this for the built-in modules (docs: Replace "umodule" with "module" everywhere. micropython#5377, py/objmodule.c: Implement default weak links for all ports. micropython#5229, and more recently RFC: Built-in module extending and removing weak links / umodules micropython#9018).

  2. Packaging. See top: Replace setup.py/metadata.txt with manifest.py. #506 (and tools/manifestfile.py: Add a library for working with manifests. micropython#8914) for the background here, but we're currently in the middle of trying to figure out how packaging works going forward, and in particular trying to unify the different ways that packages can be installed/deployed to devices.
    The summary version is that for MicroPython, pypi adds a lot of complexity and potentially doesn't need to be part of how we install packages though any of the various mechanisms (on-device, via usb/uart, mass storage, or freezing).
    So, I'd be curious to hear your thoughts on this, especially in terms of your intentions with this package and pypi and how it's used with microdot, and in particular whether you plan to use it from CPython.

@miguelgrinberg
Copy link
Contributor Author

miguelgrinberg commented Aug 5, 2022

  1. a) the problem with upyjwt is that this will also become the import name, since this is a one file module. For that reason I think the name should be ujwt.
    b) I really have no horse in this race, but I always thought it was a good idea to distinguish these minimalistic libraries from their CPython ones by adding the "u" prefix. In my view the "u" means you are using a simplified version. But I'm okay to rename it if that is what you prefer.
  2. I made sure the code runs on CPython, but I do not intend for it to be required nor used on that platform, since it makes more sense to go with the standard PyJWT. Would you prefer that I remove the packaging files and add a manifest.txt file instead? It's fine by me, but this is not implemented yet. It may make more sense to commit as is, and then add the refactor to this other PR, maybe?

@andrewleech
Copy link
Contributor

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 u/no-u question is a complicated one, in the past I too have thought it's "helpful" to differentiate the cut down module, however it complicates compatibility.

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.
It also just adds work for anyone porting examples / libraries that use jwt, meaning you'll need to fork them even just to change the import lines.

I'm all in favour of dropping the u when the implemented library is a compatible subset of the upstream library API.

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...

@jimmo
Copy link
Member

jimmo commented Aug 6, 2022

OK thank you @miguelgrinberg & @andrewleech

I really have no horse in this race, but I always thought it was a good idea to distinguish these minimalistic libraries from their CPython ones by adding the "u" prefix. In my view the "u" means you are using a simplified version. But I'm okay to rename it if that is what you prefer.

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").

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?

Yes I was going to suggest exactly this. So we'd have python-ecosys/pyjwt/jwt.py

So on micropython you'd do >>> upip.install('pyjwt') or (soon) $ mpremote install pyjwt, and then code would import jwt, matching the Python experience as closely as possible.

@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).

@miguelgrinberg
Copy link
Contributor Author

@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!

@jimmo
Copy link
Member

jimmo commented Aug 8, 2022

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 !

@jimmo jimmo closed this Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interest in a ujwt library?
3 participants