|sti_object.becomes(Parent) unexpectedly mutating the receiver||Peter Brown||7/20/12 2:48 PM|
I ran into an interesting issue today with ActiveRecord's becomes method and discovered that it is mutating the receiver without me knowing it.
The API docs say
"The new instance will share a link to the same attributes as the original class. So any change to the attributes in either instance will affect the other."
However, it doesn't say that the type attribute is changed on the receiver just by the method call.
Should the docs be updated to say that the receiver's type attribute will be changed to the class it becomes, or should becomes be changed so that it doesn't automatically mutate the receiver? Either option would be an easy fix, though the latter would break backwards compatibility. I am using becomes with things like form_for and content_tag_for so I'm using the new object returned by becomes as opposed to the mutated object.
Here's an example of the undocumented behavior I was seeing:
class Parent < ActiveRecord::Base
class Child < Parent
child = Child.new
child.type # => 'Child'
new_child = child.becomes(Parent)
child.type # => 'Parent'
new_child.type # => 'Parent'
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Matt Jones||7/20/12 3:45 PM|
'type' is an attribute; it gets changed - and the docs say the changes will happen to both. A bit unclear, but not a bug.
Definitely worth an update to the documentation, though.
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Peter Brown||7/20/12 5:44 PM|
Just stumbled upon a pull request from last year with some discussion and it seemed like people were generally in favor of changing the behavior. I'd be willing to bring it back to life if people are still interested in it.
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Dheeraj Kumar||7/20/12 7:22 PM|
Got bit by this a couple of weeks ago. +1 for the merge.
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Nicolás Sanguinetti||7/21/12 12:48 PM|
-1 for the merge.
+1 for a doc fix where this is more explicit.
I've always used x.becomes(Foo) for the side effect (mutating x)
instead of for the return value.
As I see it, the name _becomes_ clearly states that the receiver is
affected. If it was #convert or #cast or something that would
reasonably return a new object while not mutating the receiver I could
see how this could be thought of as a bug.
But it just makes sense that #becomes alters the original object.
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Peter Brown||7/21/12 1:44 PM|
The reason I disagree with becomes mutating the object in place is because it's inconsistent with almost all of Ruby and Rails' APIs. Methods that mutate an object (or raise an exception) end with !. Whether it's a string, hash, or array, methods that mutate these objects have two versions. There are exceptions such as String#clear, but compared to PHP, consistency is something Ruby and Rails have going for them.
Why diverge from this pattern for a single case? If becomes is not the method for this, them maybe there should be an alternative like object.behave_like(Class)
On Saturday, July 21, 2012 3:48:08 PM UTC-4, Nicolás Sanguinetti wrote:
-1 for the merge.
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Steve Klabnik||7/21/12 2:01 PM|
> Methods thatThis isn't correct. "dangerous" methods end with !. It has nothing to
do with mutation.
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Peter Brown||7/21/12 3:47 PM|
I apologize, I wasn't trying to turn this into a debate about semantics. Steve, does that mean you're in favor of updating the docs too then? If so, any interest in adding a version that doesn't have side effects?
|Re: [Rails-core] sti_object.becomes(Parent) unexpectedly mutating the receiver||Steve Klabnik||7/21/12 3:53 PM|
I don't use this feature, so I don't have an opinion about the
non-side effect version, but I don't see how changing the docs to make
this behavior explicit would hurt anything.