Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make AR::Base#changed_attributes to return indifferent hash #10589

Closed
wants to merge 1 commit into from

7 participants

Bogdan Gusiev Steve Klabnik Carlos Antonio da Silva Bert Goethals Arthur Nogueira Neves Rafael Mendonça França Aditya Kapoor
Bogdan Gusiev

changed_attributes hash with symbols as keys has better semantics.

Aditya Kapoor

It is required to return HashWithIndifferentAccess instead of just plain regular Hash???
I mean what value does it bring in to the code???

Steve Klabnik
Collaborator

changed_attributes hash with symbols as keys has better semantics.

Can you elaborate on this?

Bogdan Gusiev

Well I expect Symbol to mean a method name or a constant and String to mean free text.

In this case hash keys are referencing a method, so symbols looks better.
You don't write has_many "posts", right? Because :posts is a method.

Carlos Antonio da Silva

I don't think we should make it indifferent access, since AR attributes also doesn't return one:

>> Post
=> Post(id: integer, title: string, body: text, created_at: datetime, updated_at: datetime)
>> Post.first.attributes
=> {"id"=>1, "title"=>"OMG", "body"=>nil, "created_at"=>Wed, 26 Jun 2013 00:53:24 UTC +00:00, "updated_at"=>Wed, 26 Jun 2013 00:53:24 UTC +00:00}
>> Post.first.attributes[:title]
=> nil
>> Post.first.attributes['title']
=> "OMG"

To me it's just consistent to be all strings. Thanks @bogdan

@rafaelfranca thoughts?

Bert Goethals

This also makes sense to me. Consistency is a good thing.

However, do we have an idea on performance impact? I don't imagine it'll be much, but it would be good to know.

Arthur Nogueira Neves
Collaborator

agree with @carlosantoniodasilva ... No need to make this with indifferent access.

We should close this IMO.

Bogdan Gusiev bogdan deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
2  activemodel/lib/active_model/dirty.rb
View
@@ -139,7 +139,7 @@ def previous_changes
# person.name = 'robert'
# person.changed_attributes # => {"name" => "bob"}
def changed_attributes
- @changed_attributes ||= {}
+ @changed_attributes ||= HashWithIndifferentAccess.new
end
private
6 activemodel/test/cases/dirty_test.rb
View
@@ -125,4 +125,10 @@ def save
assert_equal ["Otto", "Mr. Manfredgensonton"], @model.name_change
assert_equal @model.name_was, "Otto"
end
+
+ test "changed_attributes" do
+ @model.name = "Otto"
+ assert_equal nil, @model.changed_attributes["name"]
+ assert_equal nil, @model.changed_attributes[:name]
+ end
end
Something went wrong with that request. Please try again.