Implement CurrentAttributes with AttributeMethods
#50676
Conversation
Backport `Active{Model,Support}::AttributeMethods`
---
First, migrate `ActiveModel::AttributeMethods` (along with its
corresponding test coverage) to `ActiveSupport::AttributeMethods`.
Next, implement `ActiveModel::AttributeMethods` in terms of
`ActiveSupport::AttributeMethods`, including a configuration to retain
`ActiveModel::MissingAttributeError`.
Implement `ActiveSupport::CurrentAttributes` with `ActiveSupport::AttributeMethods`.
---
Replace Code Generator blocks with
`ActiveSupport::AttributeMethods`-generated methods like
`attribute(name)` and `attribute=(name, value)`.
There was a problem hiding this comment.
As an alternative to introducing this as a concern that's part of the public API, we could mark this :nodoc: to treat it as an implementation detail of ActiveModel::AttributeMethods.
| ActiveSupport::CodeGenerator.batch(generated_attribute_methods, __FILE__, __LINE__) do |owner| | ||
| names.each do |name| | ||
| owner.define_cached_method(name, namespace: :current_attributes) do |batch| | ||
| batch << | ||
| "def #{name}" << | ||
| "attributes[:#{name}]" << | ||
| "end" | ||
| end | ||
| owner.define_cached_method("#{name}=", namespace: :current_attributes) do |batch| | ||
| batch << | ||
| "def #{name}=(value)" << | ||
| "attributes[:#{name}] = value" << | ||
| "end" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| ActiveSupport::CodeGenerator.batch(singleton_class, __FILE__, __LINE__) do |owner| | ||
| names.each do |name| | ||
| owner.define_cached_method(name, namespace: :current_attributes_delegation) do |batch| | ||
| batch << | ||
| "def #{name}" << | ||
| "instance.#{name}" << | ||
| "end" | ||
| end | ||
| owner.define_cached_method("#{name}=", namespace: :current_attributes_delegation) do |batch| | ||
| batch << | ||
| "def #{name}=(value)" << | ||
| "instance.#{name} = value" << | ||
| "end" | ||
| end | ||
| end | ||
| end | ||
| names.each { |name| define_attribute_methods name } |
There was a problem hiding this comment.
@byroot could you review these changes, since you're in this block's git blame? I'm curious about how to re-implement the delegation method definitions. At the moment, the tests pass, but rely on the first #method_missing to handle the delegation.
There was a problem hiding this comment.
At the moment, the tests pass, but rely on the first
#method_missingto handle the delegation.
YOu mean from the class to the instance? Yeah define method (or eval) inside method_missing is a terrible pattern I wish to eliminate.
I'll set a reminder to look at your PR tomorrow.
There was a problem hiding this comment.
Ah, but we were eagerly defining the delegators for the attributes, it's only for unexpected method that we lazy defined them (which is less bad but not great).
I think you can simply call delegate right after the line we're commenting, and you can use the private as: argument to get optimized delegation, should produce about the same result.
There was a problem hiding this comment.
@byroot I think I understand your suggestion in principle, but I'm struggling to craft the statement.
I'm trying this, but it's failing:
delegate name, "#{name}=", to: :instance, as: singleton_class
# => activesupport/lib/active_support/core_ext/module/delegation.rb:220:in `public_instance_method': undefined method `world' for class `#<Class:CurrentAttributesTest::Current>' (NameError)
#
# receiver_class.public_instance_method(method)
# ^^^^^^^^^^^^^^^^^^^^^^^There was a problem hiding this comment.
I'm trying this, but it's failing:
as: take the class of the object that is supposed to receive the call, so here it would have been as: self.
There was a problem hiding this comment.
Actually if you still to want to reduce duplication in that module, you can do the delegate part only, that would do it.
|
Thank you for the pull request. I don't think this change makes sense. The code that is supposedly duplicated is smaller than the entirety of the new I don't think this extraction is worth. |
Follow-up to [rails#50676][] Instead of relying on code generation, call a corresponding [delegate][] method on the `.singleton_class`. [rails#50676]: rails#50676 [delegate]: https://edgeapi.rubyonrails.org/classes/Module.html#method-i-delegate
Follow-up to [rails#50676][] Instead of relying on code generation, call a corresponding [delegate][] method on the `.singleton_class`. [rails#50676]: rails#50676 [delegate]: https://edgeapi.rubyonrails.org/classes/Module.html#method-i-delegate
Follow-up to [rails#50676][] Instead of relying on code generation, call a corresponding [delegate][] method on the `.singleton_class`. [rails#50676]: rails#50676 [delegate]: https://edgeapi.rubyonrails.org/classes/Module.html#method-i-delegate Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
Motivation / Background
The
ActiveSupport::CurrentAttributesimplementation copies a lot of the same code provided by theActiveModel::AttributeMethodsconcern.Backport
Active{Model,Support}::AttributeMethodsFirst, migrate
ActiveModel::AttributeMethods(along with its corresponding test coverage) toActiveSupport::AttributeMethods.Next, implement
ActiveModel::AttributeMethodsin terms ofActiveSupport::AttributeMethods, including a configuration to retainActiveModel::MissingAttributeError.Implement
ActiveSupport::CurrentAttributeswithActiveSupport::AttributeMethods.Replace Code Generator blocks with
ActiveSupport::AttributeMethods-generated methods likeattribute(name)andattribute=(name, value).Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]