Avoid 'last arg as keyword param' warning when building user middleware on Ruby 2.7 #1153
Conversation
|
I'm not sure about the tests, I don't see a simple way to check if the warning was not emitted. I'm open to any suggestions about how to test this change more thoroughly! |
|
I'm expecting the |
|
Thanks for working on this! Splendid find. There's a very small gem which acts as a shim around the method: https://github.com/ruby/ruby2_keywords We can start depending on that gem, to solve this in a compatible (and very widespread) way. What do you think about that? |
|
I think that sounds like a great idea! I was worried about adding a new gem dependency when the implementation was trivial, no problem for me to switch over to it. |
dgholz
force-pushed
the
ruby_2.7.0_warnings_from_middleware_construction
branch
from
5590aca
to
9aa0608
Compare
|
(Right, and then the new local variables need to be used, as well.) |
|
Thanks for your patience! |
|
I appreciate the PR and your patience. |
|
I did not notice that other PR! Thanks for the comment, I'll add |
This change marks the methods which construct user middleware, so they are
called with the 2.6 keyword argument behaviour in 2.7 (and avoid printing a
warning about it).
Some users add their own middleware & write their own constructors with named
parameters, and use keyword arguments when adding their middleware to the
builder:
```
class MyMiddleware < Faraday::Middleware
def initialize(app, my_argument:)
@app = app
@my_argument = my_argument
end
[...]
connection = Faraday.new() do |configuration|
configuration.use MyMiddleware, my_argument: 'hello'
end
```
This works great in 2.6 & 2.7, but 2.7 prints a deprecation warning:
```
/Users/danielholz/.gem/ruby/2.7.0/gems/faraday-1.0.1/lib/faraday/dependency_loader.rb:21: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/danielholz/code/some_project/lib/some_project/faraday_middleware.rb:2: warning: The called method `initialize' is defined here
```
The warning is a bit frustrating, since the calling code is not in the user's
code, so they cannot add `**` to the call. The best option for them is to
change their middleware to accept a Hash as a positional parameter & pass a
Hash when adding their middleware (and checking for required parameters
themselves).
I tried an alternative to `ruby2_keywords` by adding an explicit `**kwargs`
argument to `Faraday::RackBuilder#use`, `Faraday::RackBuilder#use_symbol`,
`Faraday::RackBuilder::Handler#initialize`, and
`Faraday::DependencyLoader#new`, but it hit the explicit delegation of keyword
arguments issue
(https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/#why-deprecated)
in 2.6.
Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>
dgholz
force-pushed
the
ruby_2.7.0_warnings_from_middleware_construction
branch
from
5ce2de1
to
385cd6d
Compare
…b classes in spec
|
I removed the test case that exercised middleware that didn't use named parameters , since it worked fine in 2.7 & 2.7. I feel like the tests are a bit verbose and repetitive, and the last few look awfully similar to the |
|
@dgholz sorry, it took me some time to find the possible cause by looking at the code. |
We don't have a specific base class for Request middleware like we have for response ones (Faraday::Response::Middleware), so cat_request should probably inherit from Faraday::Middleware instead.
|
Thank you @dgholz for the effort! And thanks @olleolleolle for the last push 😉 |
|
I'm not a fan of having another dependency or the added complexity.
|
|
@grosser Hi! Glad for the feedback! We did this change in order to avoid warnings. If we can avoid warnings in some other way (such as those you listed), it'd be neat.
|
|
ruby 3 is a great idea so we don't run into unexpected errors of any new
hackery :)
…On Mon, Oct 19, 2020 at 11:22 PM Olle Jonsson ***@***.***> wrote:
@grosser <https://github.com/grosser> Hi! Glad for the feedback!
We did this change in order to avoid warnings. If we can avoid warnings in
some other way (such as those you listed), it'd be neat.
- Any existing-middleware-compatible improvement is welcome
- CI runs 2.7.2 already, so a removal could be attempted.
- I added a feature request: Ruby 3 in the CI matrix
<#1189>, to see if
anything bad would happen with the upgrade.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1153 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACYZ4OF6C3XUCKHLJC6B3SLUUBXANCNFSM4MXF7TSQ>
.
|
This change marks the methods which construct user middleware, so they are called with the 2.6 keyword argument behaviour in 2.7 (and avoid printing a warning about it).
Some users add their own middleware & write their own constructors with named parameters, and use keyword arguments when adding their middleware to the builder:
This works great in 2.6 & 2.7, but 2.7 prints a deprecation warning:
The warning is a bit frustrating, since the calling code is not in the user's code, so they cannot add
**to the call. The best option for them is to change their middleware to accept a Hash as a positional parameter & pass a Hash when adding their middleware (and checking for required parameters themselves).I tried an alternative to
ruby2_keywordsby adding an explicit**kwargsargument toFaraday::RackBuilder#use,Faraday::RackBuilder#use_symbol,Faraday::RackBuilder::Handler#initialize, andFaraday::DependencyLoader#new, but it hit the explicit delegation of keyword arguments issue (https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/#why-deprecated) in 2.6.