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

Clarify why model[] is preferred over read_attribute #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarcPer
Copy link

@MarcPer MarcPer commented Aug 10, 2022

Addresses #155

@MarcPer MarcPer changed the title Master Clarify why model[] is preferred over read_attribute Aug 10, 2022
@pirj pirj requested a review from a team October 20, 2022 21:50
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

A small note that to be fair, you can pass a block to read_attribute with a handler when the attribute is missing, an this is exactly what [] does.

@MarcPer
Copy link
Author

MarcPer commented Oct 21, 2022

@pirj that's a good point, I can adapt the PR to take this into account. How about this?

Prefer self[:attribute] over read_attribute(:attribute), if the latter is used without a block. self[] raises an ActiveModel::MissingAttributeError if the attribute is missing, whereas read_attribute does not.

[source,ruby]
----
# bad
def amount
  read_attribute(:amount) * 100
end

# good
def amount
  self[:amount] * 100
end

def amount
  read_attribute(:amount) { 0 } * 100
end
----

@pirj
Copy link
Member

pirj commented Oct 21, 2022

I think this is unnecessary, the PR is good as is.
The point in preferring [] is to fail during the development phase making typos in attribute names obvious. I can't think of a real-world usage of a useful read_attribute fallback when the attribute doesn't really exist on the model.

@MarcPer
Copy link
Author

MarcPer commented Oct 21, 2022

After looking more into the Rails code, I found out the conclusion from the referenced discussion is not exactly correct.

If I just try any random attribute with [], I don't get an exception raised:

model[:wrong_attr]
# => nil

The MissingAttributeError exception comes in when an attribute exists in the model, but is not initialized, as written in the comments before the method definition:

person = Person.select('id').first
person[:name]            # => ActiveModel::MissingAttributeError: missing attribute: name

The important thing to realize is that the exception is only raised when Person has a name column. Otherwise the result would be nil. Given that, I'd say the text I introduced in this PR is misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants