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

Fix delegation in ActiveModel::Type.lookup #42289

Merged
merged 2 commits into from May 25, 2021

Conversation

eregon
Copy link
Contributor

@eregon eregon commented May 25, 2021

Summary

  • Without the change the new test fails like this:
  Failure:
  ActiveModel::TypeTest#test_registering_a_new_type [test/cases/type_test.rb:21]:
  Expected: #<struct args={}>
    Actual: #<struct args=nil>

Other Information

Similar to #42266, similar issue, similar fix.

* Without the change the new test fails like this:
  Failure:
  ActiveModel::TypeTest#test_registering_a_new_type [test/cases/type_test.rb:21]:
  Expected: #<struct args={}>
    Actual: #<struct args=nil>
* (*args, **kwargs)-delegation is not correct on Ruby 2.7 unless the
  target always accepts keyword arguments (not the case for `Struct.new(:args).new`).
  See https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html
* (...) is already used in several places in Rails and looks nicer.
@eregon
Copy link
Contributor Author

eregon commented May 25, 2021

I switched to (...) since that works in this case, and it's already used in Rails.

I also took a quick look at other potential delegation issues in ActiveModel with git grep -P '\*.*,.*\*\*' **/*.rb and that only shows lib/active_model/callbacks.rb which seems mostly OK (the target method expects keyword arguments, although not directly with **).

@kamipo kamipo merged commit 328113b into rails:main May 25, 2021
@p8
Copy link
Member

p8 commented May 30, 2021

@eregon Does this change require at least Ruby 2.7.3?

@eregon
Copy link
Contributor Author

eregon commented May 31, 2021

@p8 No, 2.7.0 already has (...).
2.7.3 adds support for (leading, arg, ...) which 2.7.0 - 2.7.2 does not have, but that's not used here.

@p8
Copy link
Member

p8 commented May 31, 2021

@eregon Thanks! That's good to know.

kamipo added a commit that referenced this pull request Jul 24, 2021
…-lookup

Fix delegation in ActiveModel::Type.lookup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants