clowne-rb / clowne

A flexible gem for cloning models
https://clowne.evilmartians.io
MIT License
316 stars 18 forks source link

Combine actions registry and resolvers store #6

Closed palkan closed 6 years ago

palkan commented 6 years ago

Finally, I've refactored (and hopefully simplified) the logic behind actions order and resolvers.

Now we have only 2 public APIs to extend adapters:

Clowne::Declarations.add :finalize, Clowne::Declarations::Finalize
Clowne::Adapters::ActiveRecord.register_resolver(
  :association,
  Clowne::Adapters::ActiveRecord::Association,
  before: :nullify
)
ssnickolay commented 6 years ago

and hopefully simplified

I think it's really easier than it was before!

But now I'm a bit confused by another question...I added two tests (https://github.com/palkan/clowne/pull/6/commits/b7c0013c962b4dacb9496dc23a4ae9be4530daef) - first green and second red. As you see, we have two different behaviour for DSL

I know, this PR about order of resolvers (not about DSL), but for users it will look like this :(

So, IMHO we need to add more information to README about DSL execution order.

p/s/ we can create issue for this work (and I will delete red test with "push force commit")

palkan commented 6 years ago

Let's do the following:

And than the red test should become green)

ssnickolay commented 6 years ago

Hmm... 🤔

Get rid of include_all

I agree 👍

Make ... position independent

It's will break overriding with traits like this

class A < Clowne::Cloner
  exclude_association :items

  trait :with_items do
    include_association :items
  end
end

A.call(record) # without items
A.call(record, traits: with_items) # with items

But I don't know how useful this is. If you don't use this "overriding" feature in your projects may be we can drop it and do as you suggested.

palkan commented 6 years ago

In this example:

class A < Clowne::Cloner
  exclude_association :items

  trait :with_items
    include_association :items
  end
end

top-level exclude_association doesn't make sense, 'cause we do not have include_all.

We can enhance the logic and add include_association! (force version); if it make sense, no sure.

So, exclude only make sense if you've already include something. That's why traits are useful, btw–they help to control inclusion/exclusion.

ssnickolay commented 6 years ago

top-level exclude_association doesn't make sense

Damn, you are right 😕 I wanted to set an example with include_all аnd exclude\include in traits, but if we get rid of include_all this also does not make sense...

(force version); if it make sense, no sure.

not_today_game_of_thrones.jpg

So,

Make include_association / exclude_association position independent (by using smth like 2P-Set

It's really good decision :+1:

palkan commented 6 years ago

@ssnickolay I got rid of include_all and made include_association/exclude_association order independent.

If you don't mind I'll merge this PR, and we'll be ready to finish with the Sequel integration.