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

Give GeneratedAttributeMethods module a name #35595

Conversation

shioyama
Copy link
Contributor

@shioyama shioyama commented Mar 13, 2019

Summary

Currently, inspecting the ancestors of an AR class gives you something like this:

=> [User (call 'User.connection' to establish a connection),
 User::GeneratedAssociationMethods,
 #<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x000055ace0f05b08>,
 ApplicationRecord(abstract),
 ApplicationRecord::GeneratedAssociationMethods,
 #<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x000055ace093c460>,
 ActiveRecord::Base,
 [...]

The second and fifth objects that appear in this list (after the model class itself) are instances of the ActiveRecord::AttributeMethods::GeneratedAttributeMethods module builder, included into every AR class to hold the class' attribute methods. These are extremely important modules for AR classes, probably the most important type of module in their ancestors, and they appear very prominently at the top of this list. But in their current form, most Rails developers probably have no idea what they are actually doing or why they are even there.

This is not helped by the fact that the generated association methods module is given its own name under the model namespace, in the case above, User::GeneratedAssociationMethods.

This PR brings some consistency and clarity to this situation by creating a constant under the model namespace to capture these generated attribute methods, so that the ancestor list above becomes:

=> [User (call 'User.connection' to establish a connection),
 User::GeneratedAssociationMethods,
 User::GeneratedAttributeMethods,
 ApplicationRecord(abstract),
 ApplicationRecord::GeneratedAssociationMethods,
 ApplicationRecord::GeneratedAttributeMethods,
 ActiveRecord::Base,
 [...]

Now, rather than an instance of a module builder class, the AR model gets a (private) named module User::GeneratedAttributeMethods, which is much more readable and approachable than before, and which follows the same pattern as User::GeneratedAssociationMethods. Also, since this applies to all AR models, the parent class' attribute methods also appear in this form as ApplicationRecord::GeneratedAttributeMethods, and we eliminate all anonymous instances in model ancestors.

Other Information

In addition to the change above, I have also renamed the previously-named ActiveRecord::AttributeMethods::GeneratedAttributeMethods to ActiveRecord::AttributeMethods::GeneratedAttributeMethodsBuilder, to emphasize that this is not a module but a module builder (class). Currently this is a point of inconsistency between generated association methods and generated attribute methods, which surely causes additional confusion to developers trying to understand how these modules work internally.

Currently GeneratedAttributeMethods is a module builder class, an
instance of which is included in every AR class. OTOH,
GeneratedAssociatedMethods is assigned to a constant under the model
namespace. This is inconsistent and looks strange in the list of
ancestors.

There is no particular reason *not* to assign a constant for this (very
important) module under the model namespace, so that's what this commit
does.

Previous to this change, ancestors for an AR class looked like this:

```
=> [User (call 'User.connection' to establish a connection),
 User::GeneratedAssociationMethods,
 #<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x000055ace0f05b08>,
 ApplicationRecord(abstract),
 ApplicationRecord::GeneratedAssociationMethods,
 #<ActiveRecord::AttributeMethods::GeneratedAttributeMethods:0x000055ace093c460>,
 ActiveRecord::Base,
 ...
```

With this change, they look like this:

```
=> [User (call 'User.connection' to establish a connection),
 User::GeneratedAssociationMethods,
 User::GeneratedAttributeMethods,
 ApplicationRecord(abstract),
 ApplicationRecord::GeneratedAssociationMethods,
 ApplicationRecord::GeneratedAttributeMethods,
 ActiveRecord::Base,
 ...
```

The previously named `GeneratedAttributeMethods` module builder class is
renamed `GeneratedAttributeMethodsBuilder` to emphasize that this is not
a module but a class.
@shioyama
Copy link
Contributor Author

I can add a changelog to this, just let me know.

@shioyama
Copy link
Contributor Author

Related: #30836.

@shioyama
Copy link
Contributor Author

@matthewd Pinging you because you've reviewed my PRs in the past, hope that's appropriate. This is a very small change, mostly cosmetic but I think, like #30836, important for code readability reasons.

Could you take a quick look? I'm actually planning to reference this in my RailsConf talk so would be great to get feedback! 😅

@rafaelfranca rafaelfranca merged commit 3dee96e into rails:master Mar 20, 2019
@shioyama
Copy link
Contributor Author

@rafaelfranca Thanks for the quick merge!!!

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

2 participants