lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.72k stars 972 forks source link

Forward env to Authorization#header_from ? #1449

Closed swatosh closed 1 year ago

swatosh commented 2 years ago

Basic Info

Would a PR that forwards env to Authorization#header_from be considered? If no, I can understand wanting to limit complexity. But something along these lines (simplified to minimize the diff):

index 3eb1042..e6dc460 100644
--- a/lib/faraday/request/authorization.rb
+++ b/lib/faraday/request/authorization.rb
@@ -23,21 +23,23 @@ module Faraday
       def on_request(env)
         return if env.request_headers[KEY]

-        env.request_headers[KEY] = header_from(@type, *@params)
+        env.request_headers[KEY] = header_from(@type, env, *@params)
       end

       private

       # @param type [String, Symbol]
+      # @param env [Faraday::Env]
       # @param params [Array]
       # @return [String] a header value
-      def header_from(type, *params)
+      def header_from(type, env, *params)
         if type.to_s.casecmp('basic').zero? && params.size == 2
           Utils.basic_header_from(*params)
         elsif params.size != 1
           raise ArgumentError, "Unexpected params received (got #{params.size} instead of 1)"
         else
           value = params.first
+          value = value.call(env) if (value.is_a?(Proc) && value.arity == 1) || (value.respond_to?(:call) && value.method(:call).arity == 1)
           value = value.call if value.is_a?(Proc) || value.respond_to?(:call)
           "#{type} #{value}"
         end

Specs and passing rubocop would accompany a real PR if something along these lines would be considered. Where would be the best place to update documentation?

Thanks!

iMacTia commented 1 year ago

Oh, this would then be used in the authorization value proc, I see! Good with me, I'd be happy to review a PR with this change 👏.

Quick early feedback: I think testing value.arity when value.is_a?(Proc) is unnecessary, as procs don't throw an error when passed more or fewer args than they expect (only lambdas should).

swatosh commented 1 year ago

Cool! I'll start working on it for real then.

Thing is, I don't think anything will prevent a user from passing a lambda in, and it will return true for .is_a?(Proc). I appreciate the feedback, I'm pretty unhappy with how that looks ATM even though I left deliberately left it ugly for the diff to be smaller.

iMacTia commented 1 year ago

Good shout, I always forget about that! Thanks for suggesting this addition 🙏

swatosh commented 1 year ago

Thank you so much! Closing now, PR as I'm able