all exceptions inherit from PyJwtError

[dradetsky]

I was trying to use pyjwt in some (connexion/flask) apps. I wanted to have all the jwt-related errors be converted into something (a connexion.exceptions.ProblemException subclass) which my app could recognize as an auth failure & thus respond with 401. Maybe some of the exceptions should be treated as 5xx (I haven't actually tried to figure out exactly what they all mean yet), but nevermind.

It's not obvious from the API dox or even the code how to catch all the pyjwt exceptions. So I had what turn out to be the two base classes inherit from PyJwtError and made a comment explaining the purpose.

If this was my repo, I'd make the comment say "All exceptions MUST inherit from PyJwtError" and reject any pr which adds an exception which doesn't, but it's not so that seems kinda impertinent.

[coveralls]

Coverage remained the same at 100.0% when pulling d77dc91 on dradetsky:exception-base-class into 54a39ae on jpadilla:master.

[jpadilla]

This is a simple enhancement I can get behind. Thanks!

[jpadilla]

Small nitpick, can we rename this to PyJWTError? Also, we'll need to expose this in pyjwt/__init__.py.

[dradetsky]

for you? of course

[mark-adams]

I'm not so sure I like this. All of our exceptions already inherit from a single base exception. The name change just breaks the public API for no real reason. What about adding this exception and making InvalidTokenError inherit from it?

…

On Tue, Mar 27, 2018, 19:29 José Padilla ***@***.***> wrote: ***@***.**** requested changes on this pull request. This is a simple enhancement I can get behind. Thanks! ------------------------------ In jwt/exceptions.py <#340 (comment)>: > @@ -1,4 +1,11 @@ -class InvalidTokenError(Exception): +class PyJwtError(Exception): Small nitpick, can we rename this to PyJWTError? Also, we'll need to expose this in pyjwt/__init__.py. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#340 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc1HhVWm1B5wdzReuecW8XZKBaSoCBSks5titlLgaJpZM4S9scC> .

[mark-adams]

Whoops! The diff was not apparent from my email and I now see that it does the EXACT thing I requested in my previous comment. 🤦‍♂️

…

On Tue, Mar 27, 2018, 19:35 Mark Adams ***@***.***> wrote: I'm not so sure I like this. All of our exceptions already inherit from a single base exception. The name change just breaks the public API for no real reason. What about adding this exception and making InvalidTokenError inherit from it? On Tue, Mar 27, 2018, 19:29 José Padilla ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > This is a simple enhancement I can get behind. Thanks! > ------------------------------ > > In jwt/exceptions.py > <#340 (comment)>: > > > @@ -1,4 +1,11 @@ > -class InvalidTokenError(Exception): > +class PyJwtError(Exception): > > Small nitpick, can we rename this to PyJWTError? Also, we'll need to > expose this in pyjwt/__init__.py. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#340 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAc1HhVWm1B5wdzReuecW8XZKBaSoCBSks5titlLgaJpZM4S9scC> > . >

[jpadilla]

@mark-adams diff looks weird, but this is adding a new exception PyJWTError and updates InvalidTokenError to subclass from PyJWTError instead of Exception.

Update: Just saw your last comment!

[mark-adams]

Yeah, the email experience was suboptimal here. My fault. LGTM.

…

On Tue, Mar 27, 2018, 19:41 José Padilla ***@***.***> wrote: @mark-adams <https://github.com/mark-adams> diff looks weird, but this is adding a new exception PyJWTError and updates InvalidTokenError to subclass from PyJWTError instead of Exception. [image: screen shot 2018-03-27 at 8 40 43 pm] <https://user-images.githubusercontent.com/83319/38002262-277f570a-31ff-11e8-84ad-8dd9701d3997.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#340 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc1HvodJMOt3rYzstn7WjPX0kxG1XZ6ks5titwhgaJpZM4S9scC> .

[jpadilla]

@dradetsky thanks!

[dropwhile]

Any chance of getting a release tagged (and uploaded to pypi) with this change in it, any time soon?

[jpadilla]

@cactus v1.6.2 will be available shortly