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

Make JWT::Signature.verify return true on success #305

Closed
wants to merge 1 commit into from

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented Apr 8, 2019

Before this change it returns nil, this makes it easier to write code like

if JWT::Signature.verify(...)
  do_the_thing
end

Before this change it returns nil, this makes it easier to write code like
`if JWT::Signature.verify ...`
@sourcelevel-bot
Copy link

Hello, @bdewater! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@bdewater
Copy link
Contributor Author

@excpt I'd appreciate any feedback you'd have 🙂

@excpt excpt self-requested a review September 17, 2019 14:41
@anakinj
Copy link
Member

anakinj commented Jul 2, 2020

Im wondering if this method will ever return anything else than true and in all the other cases raise an exception?

You example could be just without the nesting.

JWT::Signature.verify(...)
do_the_thing

It's maybe not clear (and I'm not sure) that the verify method will always raise an exception when verification fails. Maybe we should make verify! an alias to that method that would indicate that it always rises when the token is not valid.

@excpt
Copy link
Member

excpt commented Jul 7, 2020

Im wondering if this method will ever return anything else than true and in all the other cases raise an exception?

You example could be just without the nesting.

JWT::Signature.verify(...)
do_the_thing

It's maybe not clear (and I'm not sure) that the verify method will always raise an exception when verification fails. Maybe we should make verify! an alias to that method that would indicate that it always rises when the token is not valid.

You are correct.

This method will always raise an exception on error conditions.

This would be an improvement for a future release since it'll break the current behaviour.

I'll schedule this one for a 3.0.0 release.

@excpt excpt added this to the Version 3.0.0 milestone Jul 7, 2020
@anakinj
Copy link
Member

anakinj commented Feb 2, 2023

Im going to close this, gotten pretty stale already. Lets try to come up with a great api for the version 3.x

@anakinj anakinj closed this Feb 2, 2023
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.

None yet

3 participants