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

Avoid 'last arg as keyword param' warning when building user middleware on Ruby 2.7 #1153

Conversation

@dgholz
Copy link
Contributor

@dgholz dgholz commented May 1, 2020

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.

@dgholz
Copy link
Contributor Author

@dgholz dgholz commented May 1, 2020

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!

@dgholz
Copy link
Contributor Author

@dgholz dgholz commented May 1, 2020

I'm expecting the Faraday::Adapter::NetHttpPersistent tests to fail, due to #1152.

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented May 1, 2020

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?

@dgholz
Copy link
Contributor Author

@dgholz dgholz commented May 4, 2020

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 dgholz force-pushed the dgholz:ruby_2.7.0_warnings_from_middleware_construction branch from 5590aca to 9aa0608 May 4, 2020
Copy link
Member

@olleolleolle olleolleolle left a comment

Awesome, we are very close!

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented May 4, 2020

(Right, and then the new local variables need to be used, as well.)

@dgholz
Copy link
Contributor Author

@dgholz dgholz commented May 4, 2020

Thanks for your patience!

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented May 4, 2020

I appreciate the PR and your patience.

@iMacTia
Copy link
Member

@iMacTia iMacTia commented May 7, 2020

The net-http-persistent issue should be fixed now, however I also noticed that this was previously attempted in #1106.
@dgholz did you notice that?

Copy link
Member

@iMacTia iMacTia left a comment

So after having a better look, I believe this PR should add the ruby2_keywords prefix to more methods, as in #1106.

As for the tests, it would be nice to see this tested properly (as I previously asked on the other PR).
You should be able to test warnings using Rspec output

@dgholz
Copy link
Contributor Author

@dgholz dgholz commented May 8, 2020

I did not notice that other PR! Thanks for the comment, I'll add ruby2_keywords to those other delegating functions and check for warnings in the test.

Daniel Holz and others added 12 commits May 1, 2020
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 dgholz force-pushed the dgholz:ruby_2.7.0_warnings_from_middleware_construction branch from 5ce2de1 to 385cd6d May 8, 2020
…b classes in spec
@dgholz
Copy link
Contributor Author

@dgholz dgholz commented May 8, 2020

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 when having two handlers test cases; but for my cases, I'm only worried about what the handler ends up building, and not at all about the order of the handlers.

@dgholz dgholz requested a review from iMacTia May 12, 2020
@iMacTia
Copy link
Member

@iMacTia iMacTia commented May 14, 2020

@dgholz sorry, it took me some time to find the possible cause by looking at the code.
I've left a comment that I believe MIGHT fix the issue.
If that still doesn't help, then please let me know and I'll probably have to checkout this locally and have a closer look.

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.
@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented May 14, 2020

I got excited by the fix, tried it locally, then applied it in a GitHub Suggestions commit, and this is ready to go, @iMacTia and @dgholz - 🎉

@iMacTia iMacTia merged commit 722821f into lostisland:master May 15, 2020
6 checks passed
6 checks passed
linting
Details
build (2.4)
Details
build (2.5)
Details
build (2.6)
Details
build (2.7)
Details
codeclimate All good!
Details
@iMacTia
Copy link
Member

@iMacTia iMacTia commented May 15, 2020

Thank you @dgholz for the effort! And thanks @olleolleolle for the last push 😉

@dgholz dgholz deleted the dgholz:ruby_2.7.0_warnings_from_middleware_construction branch May 25, 2020
@grosser
Copy link
Contributor

@grosser grosser commented Oct 19, 2020

I'm not a fan of having another dependency or the added complexity.

  • can this be removed since 2.7.2 removes these warnings ?
  • can we either force hashes or keyword args to be used and then remove this ?
@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Oct 20, 2020

@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, to see if anything bad would happen with the upgrade.
@grosser
Copy link
Contributor

@grosser grosser commented Oct 20, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.