Deprecate writing to Base#attributes#463
Conversation
| end | ||
|
|
||
| attr_accessor :attributes # :nodoc: | ||
| attr_writer :attributes # :nodoc: |
There was a problem hiding this comment.
These attributes are marked with # :nodoc:, so they're private and free from the typical rules applied to breaking changes.
Is a deprecation cycle too cautious?
Could this PR be closed without the changes to #attributes proposed in #410 being considered breaking changes?
seanpdoyle
force-pushed
the
deprecate-attributes-write
branch
from
b2bc3ad to
15cb07e
Compare
seanpdoyle
force-pushed
the
deprecate-attributes-write
branch
2 times, most recently
from
21a3b98 to
ef6c311
Compare
The Base class defines an `attr_accessor :attributes` that is marked with `# :nodoc:`. Technically, this means that any interaction with `Base#attributes` or `Base#attributes=` is not part of the public interface, and is free to be changed or removed without breaking the public API. However, given the project's age and the long-term period of time with minimal changes to the public interface, this PR proposes that there be a public deprecation of **writing** to the Hash instance returned by the `Base#attributes` method. The migration to `ActiveModel::Attributes` proposed in [rails#410][] will involve changes to the `Base#attributes` method (due to [ActiveModel::Attributes#attributes][]). Reading from the value returned by `ActiveModel::Attributes#attributes` will remain the same, but writing to that value will have no affect since the value is a Hash copy created by [ActiveModel::AttributeSet#to_hash][], and not the Hash instance used internally. Similarly, `ActiveModel::Attributes` does not expose a corresponding `#attributes=` method. By deprecating `#attributes=`, changes made that incorporate `ActiveModel::Attributes` will not need to add an otherwise unnecessary `#attributes=` implementation. Once deprecated and part of a release cycle, the `ActiveResource::AttributeSet` class can be removed, and the Active Model migration's backwards compatibility burden can be reduced. [rails#410]: rails#410 [ActiveModel::Attributes#attributes]: https://edgeapi.rubyonrails.org/classes/ActiveModel/Attributes.html#method-i-attributes [ActiveModel::AttributeSet#to_hash]: https://github.com/rails/rails/blob/v8.1.1/activemodel/lib/active_model/attribute_set.rb#L36-L39
seanpdoyle
force-pushed
the
deprecate-attributes-write
branch
from
ef6c311 to
55c7155
Compare
| def dup | ||
| self.class.new.tap do |resource| | ||
| resource.attributes = @attributes | ||
| resource.send :instance_variable_set, "@attributes", @attributes |
There was a problem hiding this comment.
This really sucks. I don't think it is worth a deprecation if we are going to start using send and instance_variable_set inside the framework code. # :nodoc: should be enough. I'll revert it
There was a problem hiding this comment.
@rafaelfranca that feedback is valid. Deprecating assignment is mainly for the sake of #410.
If #410 is not viable, this isn't worthwhile.
If #410 is viable, this might be worth reconsidering. Rather than send :instance_variable_set, would wrapping this call to #attributes= in a silence block be a good enough temporary solution so that the deprecation could be released ahead of #410 being released?
| resource.send :instance_variable_set, "@attributes", @attributes | |
| ActiveResource.deprecator.silence { resource.attributes = @attributes } |
There was a problem hiding this comment.
attributes= was never public, so I'm not sure why we need to deprecate it in order to add ActiveModel::Attributes. ActiveModel::Attributes don't define attributes=, only attributes.
Also, now attributes= is just defaulting to the method_missing behavior since it is protected. So I will deprecate it there.
There was a problem hiding this comment.
attributes=was never public,
You're right -- the attr_accessor :attributes was marked as # :nodoc:
https://github.com/rails/activeresource/blob/v6.1.4/lib/active_resource/base.rb#L1184
The idea to deprecate the accessor came from it being Ruby-level public. Since it's omitted from the public documentation, I defer to you on whether or not a deprecation cycle is necessary. Skipping the deprecation cycle would simplify things a lot.
3 participants
The Base class defines an
attr_accessor :attributesthat is markedwith
# :nodoc:. Technically, this means that any interaction withBase#attributesorBase#attributes=is not part of the publicinterface, and is free to be changed or removed without breaking the
public API.
However, given the project's age and the long-term period of time with
minimal changes to the public interface, this PR proposes that there be
a public deprecation of writing to the Hash instance returned by the
Base#attributesmethod.The migration to
ActiveModel::Attributesproposed in #410 willinvolve changes to the
Base#attributesmethod (due toActiveModel::Attributes#attributes). Reading from the value returned
by
ActiveModel::Attributes#attributeswill remain the same, butwriting to that value will have no affect since the value is a Hash copy
created by ActiveModel::AttributeSet#to_hash, and not the Hash
instance used internally.
Similarly,
ActiveModel::Attributesdoes not expose a corresponding#attributes=method. By deprecating#attributes=, changes made thatincorporate
ActiveModel::Attributeswill not need to add an otherwiseunnecessary
#attributes=implementation.Once deprecated and part of a release cycle, the
ActiveResource::AttributeSetclass can be removed, and the ActiveModel migration's backwards compatibility burden can be reduced.