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

Handle separation of positional and keyword arguments in ruby 2.7 #1106

Closed
wants to merge 2 commits into from
Closed

Handle separation of positional and keyword arguments in ruby 2.7 #1106

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 2, 2020

Ruby 2.7 separates positional and keyword arguments. This resulted in several deprecation warnings in Faraday::RackBuilder. Keyword arguments are often passed to it that are implicitly passed through as a hash (as the last positional argument). Ruby 2.7 raises a warning for this and 3.0 will error.

For example, faraday-http-cache, which is configured something like builder.use Faraday::HttpCache, serializer: Marshal, shared_cache: false, will raise warnings, because RackBuilder doesn’t handle the Ruby-2.7 change.

This PR removes the deprecation in a backwards- and forward-compatible way. It uses a new compatability method in ruby 2.7 and a shim to support older rubies, which is recommended on the ruby blog.

Gemfile Outdated Show resolved Hide resolved
Copy link
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.

This change uses a well-defined workaround gem for the keyword changes: looks good!

@iMacTia
Copy link
Member

iMacTia commented Jan 9, 2020

@olleolleolle @mcgregordan sorry for the delay, busy return from Holidays.
I'm having a look at this now!

@iMacTia
Copy link
Member

iMacTia commented Jan 9, 2020

OK, I initially thought this was unnecessary as the RackBuilder doesn't really accept keyword arguments, however I believe this change will technically allow middleware and adapters to receive keyword arguments without warnings in new and old Ruby versions.

I'd however feel a little more comfortable if we also had a test with a dummy middleware/adapter that shows this is working as expected (and since our tests run against all ruby version between 2.4 and 2.7, we will be sure this works as expected).

Is that something we can add before merging the PR?

@ghost
Copy link
Author

ghost commented Jan 9, 2020

Thanks @iMacTia. I think middleware and adapters are the crucial point. Otherwise this would be unnecessary, as you say.

Agreed on the extra test. I’ll add something over the next few days.

@iMacTia
Copy link
Member

iMacTia commented May 15, 2020

This has now been implemented and test-covered in #1153

@iMacTia iMacTia closed this May 15, 2020
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.

None yet

2 participants