Skip to content

Callable authorizers#1345

Merged
iMacTia merged 4 commits intolostisland:mainfrom
sled:feature/callable-authorizers
Dec 15, 2021
Merged

Callable authorizers#1345
iMacTia merged 4 commits intolostisland:mainfrom
sled:feature/callable-authorizers

Conversation

@sled
Copy link
Copy Markdown
Contributor

@sled sled commented Dec 12, 2021

Description

Allows callable authorizers to be passed as proposed in #1344

else
value = params.first
value = value.call if value.is_a?(Proc)
value = value.call if value.is_a?(Proc) || value.respond_to?(:call)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The || is on purpose, because checking the ancestors of an object should be faster than looking up a method. This way, there should be no performance penalty on existing code bases which use a proc already.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Love this attention to performance, thanks for caring and explaining!

Comment thread spec/faraday/request/authorization_spec.rb Outdated
Copy link
Copy Markdown
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thanks for the neat test-case and implementation!

Copy link
Copy Markdown
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

Copy link
Copy Markdown
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

LGTM 💯 🎉

else
value = params.first
value = value.call if value.is_a?(Proc)
value = value.call if value.is_a?(Proc) || value.respond_to?(:call)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Love this attention to performance, thanks for caring and explaining!

@iMacTia iMacTia merged commit 9ef407a into lostisland:main Dec 15, 2021
jrochkind pushed a commit to jrochkind/faraday that referenced this pull request Dec 20, 2021
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.

3 participants