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

Default [] on type: :array is returning the exact same 'default' object #113

Open
nathankot opened this issue Apr 21, 2014 · 9 comments
Open

Comments

@nathankot
Copy link

For example:

class FakeModel
  include MotionModel::Model
  include MotionModel::ArrayModelAdapter
  include MotionModel::Validatable

  columns test: { type: :array, default: [] }
end

@one = FakeModel.new
@two = FakeModel.new

@one.test.push(:one)

p @one.test.object_id == @two.test.object_id # => true
p @two.test.inspect # => "[:one]"

Not sure if desirable? Happy to look into this if not.

@sxross
Copy link
Owner

sxross commented Apr 21, 2014

This is a Ruby thing. Once you assign an array, I believe it will be a different object. Otherwise, you'll have to copy the array (dup). You can test this:

@two.type = ['hello']

p @one.test.object_id == @two.test.object_id # => true
# ^^^ should be false

Steve

@nathankot
Copy link
Author

Yep, although my use case is that I just want to append items directly to the
default array without having to think about whether it will affect other model
instances... wonder if its better to provide dups as the default?

Cheers,

Nathan Kot

On Mon, Apr 21, 2014 at 09:26:38AM -0700, s.ross wrote:

This is a Ruby thing. Once you assign an array, I believe it will be a different object. Otherwise, you'll have to copy the array (dup). You can test this:

@two.type = ['hello']

p @one.test.object_id == @two.test.object_id # => true
# ^^^ should be false

Steve


Reply to this email directly or view it on GitHub:
#113 (comment)

@sxross
Copy link
Owner

sxross commented Apr 21, 2014

I wonder if a more general solution might be to have the default take a value or a proc. So something like this would work:

columns test: { type: :array, default: lambda{[].dup}}

This would let you do things like generate UUIDs and stuff as defaults (!). Or is the syntax too ugly for words?

WDYT?

@nathankot
Copy link
Author

Hmmmmmmm, I think its a bit ugly xD

I wonder how ActiveRecord does it? When I used it never ran into this problem before, so I'm assuming they do it
transparently but will have a look into that today after work!

Nathan Kot

On Mon, Apr 21, 2014 at 03:44:18PM -0700, s.ross wrote:

I wonder if a more general solution might be to have the default take a value or a proc. So something like this would work:

columns test: { type: :array, default: lambda{[].dup}}

This would let you do things like generate UUIDs and stuff as defaults (!). Or is the syntax too ugly for words?

WDYT?


Reply to this email directly or view it on GitHub:
#113 (comment)

@DougPuchalski
Copy link
Contributor

Well, you wouldn't need the dup:

columns test: { type: :array, default: -> { [] } }

Perhaps it would make sense to clone a non-primitive default value when creating an instance, but then you have to decide, shallow, deep, etc. Honestly I think rails can be a little to clever sometimes, I think the above is clear, it's saying that the default should be evaluated at the time it is created. Rails moved in this direction anyway, i.e. using procs for scopes and things like that.

@sxross
Copy link
Owner

sxross commented Apr 22, 2014

I'm inclined to agree with @aceofspades on this. ActiveRecord doesn't really address mutable defaults (for lack of a better term). They defer it to after hooks. A stubby proc, lambda or method would be a clean general solution, IMO.

@nathankot
Copy link
Author

I can live with the shorthand :) wonder if it's already supported by motion model?

sxross added a commit that referenced this issue Apr 22, 2014
Addresses issue #113. Allows specification of a proc, block, or sym for default values.
@sxross
Copy link
Owner

sxross commented Apr 22, 2014

I pushed a fix for this in the proc_defaults branch (https://github.com/sxross/MotionModel/tree/proc_defaults). Please have a look.

@nathankot
Copy link
Author

perfect! looks good 👍

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

No branches or pull requests

3 participants