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

Define alias attribute methods in define_attribute_methods #49173

Conversation

nvasilevski
Copy link
Contributor

@nvasilevski nvasilevski commented Sep 6, 2023

Fixes #48931

undefine_attribute_methods now removes alias attribute methods along with attribute methods. This commit changes define_attribute_methods to redefine methods back if any alias attributes were declared which provides applications and libraries an option to bring the alias methods back after using undefine_attribute_methods.

Implementation details

In addition to existing alias_attribute internal lists we will populate one more - aliases_by_attribute_name which will keep a list of alias attributes per attribute name. We will use this list in the define_attribute_methods to restore aliases any time the method is being redefined. Basically it makes define_attribute_methods responsible for defining both alias attribute methods and actual attribute methods for most cases apart from when a non-attribute thing is being aliased.

@nvasilevski nvasilevski force-pushed the define-alias-attribute-methods-in-define_attribute_methods branch from a2c8e29 to 3eae3db Compare September 6, 2023 19:43
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor concern over that new attribute, but once this is cleared 👍

Comment on lines 207 to 209
self.attribute_aliases = attribute_aliases.merge(new_name => old_name)
self.local_attribute_aliases = local_attribute_aliases.merge(new_name => old_name)
self.aliases_by_attribute_name[old_name] = aliases_by_attribute_name[old_name] + [new_name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attribute_aliases is a class_attribute, so it's inherited and it makes sense to re-assign it.

However local_attribute_aliases and aliases_by_attribute_name are regular instance variables, so you can actually mutate them without issue.

Suggested change
self.attribute_aliases = attribute_aliases.merge(new_name => old_name)
self.local_attribute_aliases = local_attribute_aliases.merge(new_name => old_name)
self.aliases_by_attribute_name[old_name] = aliases_by_attribute_name[old_name] + [new_name]
self.attribute_aliases = attribute_aliases.merge(new_name => old_name)
self.local_attribute_aliases[new_name] = old_name
self.aliases_by_attribute_name[old_name] << new_name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I wonder if we really need to keep both. I see local_attribute_aliases is used here:

local_attribute_aliases.each do |new_name, old_name|
generate_alias_attribute_methods(code_generator, new_name, old_name)
end
, but I think you could use aliases_by_attribute_name for that (or the opposite, what I'm trying to say is that it seem to me that we don't actually need both).

@nvasilevski nvasilevski force-pushed the define-alias-attribute-methods-in-define_attribute_methods branch from 3eae3db to 248acb9 Compare September 6, 2023 20:05
`undefine_attribute_methods` now removes alias attribute methods along
with attribute methods. This commit changes `define_attribute_methods` to
redefine methods back if any alias attributes were declared which provides
applications and libraries an option to bring the alias methods back
after using `undefine_attribute_methods`.
@nvasilevski nvasilevski force-pushed the define-alias-attribute-methods-in-define_attribute_methods branch from 248acb9 to 0f5563b Compare September 6, 2023 20:06
@byroot byroot merged commit 304f0a3 into rails:main Sep 6, 2023
4 checks passed
@nvasilevski nvasilevski deleted the define-alias-attribute-methods-in-define_attribute_methods branch September 6, 2023 21:03
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.

undefine_attribute_methods now clears alias attribute methods
2 participants