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

Add RBS for [email protected] #243

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Add RBS for [email protected] #243

merged 1 commit into from
Oct 20, 2022

Conversation

autopp
Copy link
Contributor

@autopp autopp commented Oct 16, 2022

This PR add RBS for [email protected].

Currently this RBS contains definition of

  • JWT.encode and JWT.decode (incomplete)
  • Error classes

@autopp autopp requested a review from pocke as a code owner October 16, 2022 15:50

def self?.encode: (untyped payload, untyped key, ::String | _Algo algorithm, ?::Hash[::String | ::Symbol, untyped] header_fields) -> ::String

def self?.decode: (String jwt, ?untyped? key, ?bool verify, ?::Hash[Symbol, untyped] options) ?{ (Hash[::String, untyped] header, ?untyped payload) -> untyped } -> [untyped, Hash[::String, untyped]]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def self?.decode: (String jwt, ?untyped? key, ?bool verify, ?::Hash[Symbol, untyped] options) ?{ (Hash[::String, untyped] header, ?untyped payload) -> untyped } -> [untyped, Hash[::String, untyped]]
def self?.decode: (String jwt, ?untyped? key, ?boolish verify, ?::Hash[Symbol, untyped] options) ?{ (Hash[::String, untyped] header, ?untyped payload) -> untyped } -> [untyped, Hash[::String, untyped]]

How about using boolish instead of bool?
https://github.com/ruby/rbs/blob/30e800622a2d7217f822beb2693bdff16f528193/docs/syntax.md#bool-or-boolish

decode method receives anything as verify, and the argument is used only for if/unless conditions. So I think we have no reason to strict it to true or false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, you are right. Also return types of _Algo#valid_alg? and _Algo#verify should be boolish.

def verify: (data: untyped, signature: ::String, verification_key: untyped) -> bool
end

def self?.encode: (untyped payload, untyped key, ::String | _Algo algorithm, ?::Hash[::String | ::Symbol, untyped] header_fields) -> ::String
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def self?.encode: (untyped payload, untyped key, ::String | _Algo algorithm, ?::Hash[::String | ::Symbol, untyped] header_fields) -> ::String
def self?.encode: (untyped payload, untyped key, ?::String | _Algo algorithm, ?::Hash[::String | ::Symbol, untyped] header_fields) -> ::String

algorithm argument looks optional.
https://github.com/jwt/ruby-jwt/blob/b1b52504a7d7819240f2bc9a50da1a79ed03df2b/lib/jwt.rb#L21

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

@autopp
Copy link
Contributor Author

autopp commented Oct 19, 2022

@pocke Thanks for review. I reflected your comment at d0f8db2.

Copy link
Member

@pocke pocke left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for your PR!

@pocke pocke merged commit 8366cff into ruby:main Oct 20, 2022
@autopp autopp deleted the add-jwt branch October 20, 2022 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants