Improve handling of record attributes #43

Merged
merged 8 commits into from Apr 15, 2012

Projects

None yet

3 participants

@norbert
Contributor
norbert commented Apr 14, 2012

This branch removes the Attributes class and changes the handling of attributes in the Record class.

Right now there is a bug in the way define_accessors works. Because define_method is called on Record the accessor methods are defined for all instances. Using method_missing and respond_to? avoids this problem. This is related to #37 and #38.

Duplicating the values in the Attributes class seemed unnecessary. This version of Record#attributes returns a plain hash that has only the original column names as keys, which is different from the current return value. To allow "indifferent" access to attributes Record#[] now works the same as the ActiveRecord equivalent.

Although I haven't benchmarked the difference extensively, both fixing the duplication and removing define_accessors from initialize seem to cause a significant speed increase. This new implementation could still use some work but only one test had to be changed. Finally, I cleaned up some unrelated code in the process.

@infused infused merged commit e14bcf4 into infused:master Apr 15, 2012
@infused
Owner
infused commented Apr 15, 2012

Thank you, this is great work and it looks to be about twice as fast in my initial testing. What do you think could still use some work?

@norbert
Contributor
norbert commented Apr 15, 2012

Well, the use of @column_names irks me because this sets an instance variable on every record when an accessor is used. Unfortunately #index is noticeably faster than scanning @columns every time so this might have to do for now.

Thanks for the quick response!

@eloyesp
Contributor
eloyesp commented Apr 16, 2012

I like the new code, but it is a backwards incompatible change.

It was for me a headache to me know what was happening (I was ussing attributes before). I've already adapted my code to work with this new version, but I think it could be a problem if you change the API so easy.

I realy recomend to attach to Semantic Versioning a little more.

Also, I think it broke the Readme promise (but not realy sure, it depends on what version of columns name are based #schema)

table.each do |record|
  Book.create(record.attributes)
end

May be it's better to make #attributes not duplicated in the other way ({:column_name => "foo"} instead of {"COLUMNAME" => "foo"})

@norbert
Contributor
norbert commented Apr 16, 2012

@eloyesp yes, I agree with you about the upgrade path which is why I explicitly mentioned that this changed the way #attributes works and I was surprised to see this released in a patch version.

Returning the original attributes seems more natural to me so maybe we could come up with a different solution for the example you gave.

@infused
Owner
infused commented Apr 17, 2012

I was too hasty in pushing a new release. I'm going to yank version 1.7.7 for now since it breaks the attributes hash. That being said, I'm quite happy to have it gone. The AR create example that @eloyesp quoted has been the source of far too many support requests over the last several years and I would prefer to explicitly define a mapping instead of just passing in an attributes hash. Something like:

table.each do |record|
  Book.create(:title => record.title, :author => record.author)
end

Perhaps we can add a deprecation warning to #attributes and then remove it entirely in version 2.0.0. Thoughts?

@eloyesp
Contributor
eloyesp commented Apr 17, 2012

I think that the AR example is very attractive, so I think it could be changed to use the mapping you say but should not be removed.

On the #attributes method problem, I realy like it, in whatever form it exist. I think that the hash returned by this pull request is correct, but prefer the other one (with the keys as symbols). May be it's better to get both (but separated).

I currently use this hash with seed-fu.

@norbert
Contributor
norbert commented Apr 17, 2012

@infused having access to the unmodified #attributes hash was actually one of the reasons for writing this patch. The example you gave looks much better than the current one but there is no reason to remove the functionality altogether.

@eloyesp I don't get how you could be using the current hash with duplicate values. When you want a hash with symbols for keys you could do this in the new implementation:

table.each do |record|
  Hash[table.columns.map { |column| [column.underscore_name.to_sym, record.attributes[column.name]] }
end

This could be extracted to a separate method on Record but I wouldn't use it, nor would I know what to name it. Similarly, underscored_name might need a better name if the method is going to be used publicly.

About the releases, @infused, maybe you could push a new 1.7.8 version that reverts the changes in addition to removing 1.7.7 because it might cause problems for the few people who already upgraded. When this discussion is closed, is there any reason for not releasing the new code as 2.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment