Confusing API for updating attributes #242

Closed
brandonweiss opened this Issue Feb 4, 2014 · 11 comments

Comments

Projects
None yet
5 participants
@brandonweiss

I wrote some code to update a Virtus object, something like:

@form.attributes.merge(user_params)

I was surprised when my tests failed. The attributes hash was being updated correctly but the reader methods for the attributes weren't returning the right data. I checked the docs and googled a bit to try and figure out how to mass-assign attributes, and found #86. Apparently I'm supposed to do this:

@form.attributes = user_params

Now my tests pass, which is great. But I can already tell that this is going to be confusing for future-me or other people. You would never know that it's only updating attributes present in the hash from looking at that if you didn't already know that Virtus specifically designed the attributes setter to behave like that. That's very confusing. Even worse, the API that someone would logically expect to work, doesn't.

@elskwid

This comment has been minimized.

Show comment Hide comment
@elskwid

elskwid Feb 4, 2014

Collaborator

@brandonweiss - I find attributes= to be a confusing interface as well. I wouldn't want to use .merge since that seems tied to the internal implementation. What about something like .merge_attributes which means what it says but we don't have to care about what's going on in there.

Collaborator

elskwid commented Feb 4, 2014

@brandonweiss - I find attributes= to be a confusing interface as well. I wouldn't want to use .merge since that seems tied to the internal implementation. What about something like .merge_attributes which means what it says but we don't have to care about what's going on in there.

@brandonweiss

This comment has been minimized.

Show comment Hide comment
@brandonweiss

brandonweiss Feb 4, 2014

Yeah, to support .merge you'd have to move the concept of attributes to an object that behaved like enumerable. Which might not be bad. But creating a method would be far easier. .merge_attributes makes sense. Or even .update_attributes.

Yeah, to support .merge you'd have to move the concept of attributes to an object that behaved like enumerable. Which might not be bad. But creating a method would be far easier. .merge_attributes makes sense. Or even .update_attributes.

@solnic

This comment has been minimized.

Show comment Hide comment
@solnic

solnic Feb 4, 2014

Owner

I'm not sure if the current API is confusing, it's a typical mass-assignment API that other libs have. Frankly I've never seen any complaints about it until now. Virtus stores values in ivars and #attributes returns hash with attribute values so I'm not sure why anybody would expect attributes.merge to work.

We can add #update - no need for the _attributes suffix. This would make virtus objects quack more like a hash.

Owner

solnic commented Feb 4, 2014

I'm not sure if the current API is confusing, it's a typical mass-assignment API that other libs have. Frankly I've never seen any complaints about it until now. Virtus stores values in ivars and #attributes returns hash with attribute values so I'm not sure why anybody would expect attributes.merge to work.

We can add #update - no need for the _attributes suffix. This would make virtus objects quack more like a hash.

@solnic

This comment has been minimized.

Show comment Hide comment
@solnic

solnic Feb 4, 2014

Owner

btw in ROM we rely on #update when you want to use immutable objects in session so adding it to virtus would be nice.

Owner

solnic commented Feb 4, 2014

btw in ROM we rely on #update when you want to use immutable objects in session so adding it to virtus would be nice.

@solnic

This comment has been minimized.

Show comment Hide comment
@solnic

solnic Feb 4, 2014

Owner

btw 2 - I hate mass-assignment. it's been extracted into a separate module in virtus 1.0 and I'm considering turning it off by default in 2.0.

Owner

solnic commented Feb 4, 2014

btw 2 - I hate mass-assignment. it's been extracted into a separate module in virtus 1.0 and I'm considering turning it off by default in 2.0.

@mbj

This comment has been minimized.

Show comment Hide comment
@mbj

mbj Feb 4, 2014

Collaborator

btw 2 - I hate mass-assignment. it's been extracted into a separate module in virtus 1.0 and I'm considering turning it off by default in 2.0.

+1

Collaborator

mbj commented Feb 4, 2014

btw 2 - I hate mass-assignment. it's been extracted into a separate module in virtus 1.0 and I'm considering turning it off by default in 2.0.

+1

@elskwid

This comment has been minimized.

Show comment Hide comment
@elskwid

elskwid Feb 4, 2014

Collaborator

In the interest of full disclosure, I hadn't tried to use the .attributes= or looked at it closely until this issue was filed. When I see that method and it's name I expect it to replace all the attributes - as if I am setting the attributes to the values being passed in. Looking at it now it seems silly that I thought this method would reset all the attributes and populate the object with new values.

I use the mass assignment at instantiation all the time with Virtus but I never use it afterwards. This is especially true when using Virtus in Rails projects. There's such a huge proliferation of hash-like interfaces in Rails. Controller params, attributes off of AR models, etc. Virtus shines in these places because it gives structure where it is missing. (Esp in controllers!). @solnic, do you have an example of another library that uses a the .attributes= for what amounts to a partial update of an object's attributes? (I'm curious about it.)

Collaborator

elskwid commented Feb 4, 2014

In the interest of full disclosure, I hadn't tried to use the .attributes= or looked at it closely until this issue was filed. When I see that method and it's name I expect it to replace all the attributes - as if I am setting the attributes to the values being passed in. Looking at it now it seems silly that I thought this method would reset all the attributes and populate the object with new values.

I use the mass assignment at instantiation all the time with Virtus but I never use it afterwards. This is especially true when using Virtus in Rails projects. There's such a huge proliferation of hash-like interfaces in Rails. Controller params, attributes off of AR models, etc. Virtus shines in these places because it gives structure where it is missing. (Esp in controllers!). @solnic, do you have an example of another library that uses a the .attributes= for what amounts to a partial update of an object's attributes? (I'm curious about it.)

@mbj

This comment has been minimized.

Show comment Hide comment
@mbj

mbj Feb 4, 2014

Collaborator

I aggree #attributes = is a SRP and DHMB (Dont hurt my brain) violation. Lets nuke it.

Collaborator

mbj commented Feb 4, 2014

I aggree #attributes = is a SRP and DHMB (Dont hurt my brain) violation. Lets nuke it.

@solnic

This comment has been minimized.

Show comment Hide comment
@solnic

solnic Feb 4, 2014

Owner

@elskwid not sure if you're trolling me right now or what 😄

Here are two libs that have #attributes= which works like in virtus:

  • ActiveRecord
  • DataMapper

Maybe you've heard about those ;)

I started Virtus by doing copy'n'paste from dm-core so some things we no longer like are still here.

Owner

solnic commented Feb 4, 2014

@elskwid not sure if you're trolling me right now or what 😄

Here are two libs that have #attributes= which works like in virtus:

  • ActiveRecord
  • DataMapper

Maybe you've heard about those ;)

I started Virtus by doing copy'n'paste from dm-core so some things we no longer like are still here.

@elskwid

This comment has been minimized.

Show comment Hide comment
@elskwid

elskwid Feb 5, 2014

Collaborator

@solnic - Not trolling at all. I have never used that method in either library. WHAT A NEWB!

Collaborator

elskwid commented Feb 5, 2014

@solnic - Not trolling at all. I have never used that method in either library. WHAT A NEWB!

@dkubb

This comment has been minimized.

Show comment Hide comment
@dkubb

dkubb Feb 5, 2014

Collaborator

I think we're all agreed that #attributes= is a crappy interface. The only reason it exists in virtus is to provide some compatibility to the AR and DM interfaces. I agree with @solnic that it should be removed for virtus 2.0. In the meantime, for virtus 1.0, I think we're better off sticking with the current semantics.

Collaborator

dkubb commented Feb 5, 2014

I think we're all agreed that #attributes= is a crappy interface. The only reason it exists in virtus is to provide some compatibility to the AR and DM interfaces. I agree with @solnic that it should be removed for virtus 2.0. In the meantime, for virtus 1.0, I think we're better off sticking with the current semantics.

@solnic solnic added this to the 2.0.0 milestone Jul 24, 2014

@solnic solnic closed this Jul 24, 2014

@solnic solnic removed this from the 2.0.0 milestone Jul 24, 2014

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