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

ArgumentError due to missing ruby2_keywords on Ruby 3.2.0 #1479

Closed
anero opened this issue Jan 5, 2023 · 4 comments · Fixed by #1483
Closed

ArgumentError due to missing ruby2_keywords on Ruby 3.2.0 #1479

anero opened this issue Jan 5, 2023 · 4 comments · Fixed by #1483
Labels

Comments

@anero
Copy link

anero commented Jan 5, 2023

Basic Info

  • Faraday Version: 1.10.2
  • Ruby Version: 3.2.0

Issue description

When adding a middleware that receives keyword arguments in the constructor, the call from DependencyLoader#new fails because the method is not defined using ruby2_keywords. I noticed that this was first added in #1153 but then got removed in #1211, mentioning that ruby 2.7 did't display any warnings when removing ruby2_keywords.
I think that with the change on Ruby 3.2.0 (related to bugs https://bugs.ruby-lang.org/issues/18625 and https://bugs.ruby-lang.org/issues/16466) this no longer works. This is also demonstrated by running the spec/faraday/rack_builder_spec.rb specs when targeting Ruby 3.2.0.

I can submit a small PR with this change targeting the 1.x branch if that works for you.

Steps to reproduce

Run spec/faraday/rack_builder_spec.rb specs on Ruby 3.2.0.

@serggl
Copy link

serggl commented Jan 12, 2023

facing the same issue

@iMacTia
Copy link
Member

iMacTia commented Jan 12, 2023

Thank you @anero, I was reading through Ruby 3.2 release notes and figured this change might cause some issues.
At the time we decided to remove it because it wasn't causing any issues, but I agree we should now follow Ruby 3.2 requirements.

Please feel free to open a PR and address this. Ideally I'd like to fix the main branch first, and we can then backport the fix over to 1.x as I assume the same issue would affect main as well?

It goes without saying we would need a test to demonstrate this as well. I'm surprised none of our tests failed but I guess that's because we never test adding a middleware with keyword arguments

timrogers added a commit to timrogers/faraday that referenced this issue Jan 18, 2023
When adding a middleware that receives keyword arguments in the
constructor, the call from `DependencyLoader#new` fails because
the method is not defined using `ruby2_keywords`.

This adds the required `ruby2_keywords` declaration to
`DependencyLoader#new`, fixing the tests and getting Faraday v1.x
working with Ruby 3.2.0.

Fixes lostisland#1479.
iMacTia added a commit that referenced this issue Jan 18, 2023
* Run tests against Ruby 3.2

* Declare `DependencyLoader#new` with `ruby2_keywords` to fix Ruby 3.2.0

When adding a middleware that receives keyword arguments in the
constructor, the call from `DependencyLoader#new` fails because
the method is not defined using `ruby2_keywords`.

This adds the required `ruby2_keywords` declaration to
`DependencyLoader#new`, fixing the tests and getting Faraday v1.x
working with Ruby 3.2.0.

Fixes #1479.

Co-authored-by: Matt <iMacTia@users.noreply.github.com>
@timrogers
Copy link

Fixed in #1483.

@olleolleolle olleolleolle linked a pull request Jan 18, 2023 that will close this issue
@olleolleolle
Copy link
Member

And released! Closing this issue.

bolshakov pushed a commit to toptal/faraday that referenced this issue Mar 22, 2023
* Run tests against Ruby 3.2

* Declare `DependencyLoader#new` with `ruby2_keywords` to fix Ruby 3.2.0

When adding a middleware that receives keyword arguments in the
constructor, the call from `DependencyLoader#new` fails because
the method is not defined using `ruby2_keywords`.

This adds the required `ruby2_keywords` declaration to
`DependencyLoader#new`, fixing the tests and getting Faraday v1.x
working with Ruby 3.2.0.

Fixes lostisland#1479.

Co-authored-by: Matt <iMacTia@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants