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

Nested params #15

Closed
palkan opened this Issue Jan 26, 2018 · 14 comments

Comments

Projects
None yet
2 participants
@palkan
Copy link
Owner

palkan commented Jan 26, 2018

Currently, we had an effect of params shadowing: nested cloner could rely on the same params as the top-level ones, but they must not know about each other (more precisely, children cloners must not know about parent cloners).

We need a way to control how params are passed to associations cloners, e.g.:

# just pass params as is (current behaviour)
include_association :users, params: true

# use custom block to prepare params
include_association :users, params: ->(params) { params[:data] }

# or use a key
include_association :profile, params: :profile

# usage
UserCloner.call(user, profile: { reset: true })

I also propose to not proxy params by default to the underlying cloners.

@palkan palkan added the enhancement label Jan 26, 2018

@ssnickolay

This comment has been minimized.

Copy link
Collaborator

ssnickolay commented Jan 26, 2018

underlying cloners.

Do you mean

include_association :users ==
  include_association :users, params: false

will be as default behaviour?

@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Jan 26, 2018

Yep

@ssnickolay ssnickolay self-assigned this Feb 11, 2018

@ssnickolay ssnickolay referenced this issue Feb 11, 2018

Merged

Add control over associations params #17

6 of 6 tasks complete
@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 14, 2018

@ssnickolay

What do you think about making possible to pass the parent record as a param to associations?

I mean, smth like this:

include_association :users, params: ->(params, record) { { parent: record,  data: params[:data] } }

And maybe event pass it by default when params: true as :parent key

UPD: frankly speaking, I forgot the exact case when it could be useful)

@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 14, 2018

One more idea – multiple keys support:

include_association :profile, params: [:profile, :meta, :store]
@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 14, 2018

And the last one – global params:

# define params that passed to nested cloners
Clowne.global_parameters = [:store, :current_user]

Works when params option is not specified (or false).

Should it work with KeyProxy; I think, yes, it should.

What about BlockProxy?

UPD: I don't really like the naming) maybe, it's better to call it context_params, i.e. we define some global context for the whole execution.

@ssnickolay

This comment has been minimized.

Copy link
Collaborator

ssnickolay commented Feb 14, 2018

What do you think about making possible to pass the parent record as a param to associations?

I think we can provide two block types

# first
include_association :users, params: ->(params, record) { { parent: record,  data: params[:data] } }

# second
include_association :users, params: ->(params) { { data: params[:data] } }

# And handle them in BlockProxy
if block.arity == 1
  block.call(params)
elseif block.arity == 2
  block.call(params, record)
else
  raise 'some error'
end

And maybe event pass it by default when params: true as :parent key

I wouldn't say I truly like this idea...This may not be useful for anyone who uses params: true and option params: parent sound like a hack)
But I like this feature and I think that it will be enough to describe it in documentation. (I mean two block types).

UPD: frankly speaking, I forgot the exact case when it could be useful)

Because we can 😎

One more idea – multiple keys support:

Agree 👍

And the last one – global params

Sounds like a good task for http://cultofmartians.com (for junior\middle?) ;) Because it's not super important feature...but maybe can be useful 🤔

@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 14, 2018

I think we can provide two block types

Or we can wrap lambda in a proc:

block = if params.lambda?
   Proc.new { |*args| params.call(*args.take(params.arity)) }
else
  params
end

or with refinements)) (i like them)

refine Proc do
  def to_proc
    return self unless lambda?
    this = self
    Proc.new { |*args| this.call(*args.take(this.arity)) }
  end
end

block = params.to_proc

Sounds like a good task for http://cultofmartians.com (for junior\middle?) ;)

👍 Let's add one and release along with the blog post

@ssnickolay

This comment has been minimized.

Copy link
Collaborator

ssnickolay commented Feb 14, 2018

or with refinements)) (i like them)

I love this magic 💉

👍 Let's add one and release along with the blog post

Yep, you read my thoughts ;)

@ssnickolay

This comment has been minimized.

Copy link
Collaborator

ssnickolay commented Feb 16, 2018

@palkan I am a bit confused...

class SomeCloner < Clowne::Cloner
  include_association :users, params: :foo
end

cloned1 = SomeCloner.call(record, foo: 1, bar: 2)
cloned2 = SomeCloner.call(record, foo: {name: 'John', surename: 'Cena'}, bar: 2)

What you expected as a nested parameters? In current realisation I'm trying to pass params.fetch(:foo) to cloned association, buts it's not correct when params[:foo] not a Hash (like for cloned1)...It seems that we can implement only "slice behaviour" (params.slice(:foo) instead of params.fetch(:foo))...or some kind of hybrid option, but I'm not sure that it will be understandable.

@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 16, 2018

it's not correct when params[:foo] not a Hash

What about raising ArgumentError when nested param is not a Hash? Thus, we require it to be a Hash (we already do, implicitly–we assume that params is a Hash in Base::Association).

@ssnickolay

This comment has been minimized.

Copy link
Collaborator

ssnickolay commented Feb 16, 2018

What about raising ArgumentError when nested param is not a Hash

For multiple keys we will demand that all be a Hash?

@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 16, 2018

Hm, looks like multiple keys won't work this way( That's not filtering

class SomeCloner < Clowne::Cloner
  include_association :users, params: :foo
end

cloned2 = SomeCloner.call(record, foo: { name: 'John', surname: 'Cena' }, bar: 2)
# UserCloner will receive { name: '...', surname: '...' } as params
@ssnickolay

This comment has been minimized.

Copy link
Collaborator

ssnickolay commented Feb 16, 2018

Hm, looks like multiple keys won't work this way

We can merge values of all hashes

class SomeCloner < Clowne::Cloner
  include_association :users, params: [:foo, :bar]
end

cloned1 = SomeCloner.call(record, foo: { name: 'John', surname: 'Cena' }, bar: 2)
# raise ArgumentError

cloned2 = SomeCloner.call(record, foo: { name: 'John'}, bar: {surname: 'Cena' })
# UserCloner will receive { name: '...', surname: '...' } as params

But I'm not sure that this will be useful 🤔

@palkan

This comment has been minimized.

Copy link
Owner Author

palkan commented Feb 16, 2018

Yeah, merging is unclear. It's always possible to use custom proc, so let's not implement multiple keys handling ourselves

@palkan palkan closed this in #17 Feb 19, 2018

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