rom-rb / rom

Data mapping and persistence toolkit for Ruby
https://rom-rb.org
MIT License
2.09k stars 161 forks source link

Remove descendants tracking #142

Closed aflatter closed 9 years ago

aflatter commented 9 years ago

Opening an issue here to discuss the effects of descendants tracking and the removal thereof. Gerenally, I'd say that descendants tracking is undesirable because it's a class-level interface and hidden from the user. Currently it's used to populate the registries of relations, mappers and commands automagically.

Here's a couple of points to discuss:

The alternative to using descendants tracking is to make registry explicit. Note that this won't bother users of rom-rails or similar integration gems as they would ship with support and conventions for location and name of those classes.

require 'my_app/relations/users'
setup = ROM.setup(...)
setup.register_relation(:users, MyApp::Relations::Users)

The advantages are the removal of a fragile technique, independence of load order and setup time, less hassle in the test suite and simplification of ROM core.

/cc @elskwid @solnic @splattael: We already talked quite a bit about this. :)

solnic commented 9 years ago

Yeah I'm :+1: here esp after adding silly code that clears the descendants arrays in specs and rom-rails to_prepare.

As you point out this can be done automatically on an integration level with frameworks so it's fine. It would be worth to consider pulling that code into rom core though so that we don't have to do it all over again in every framework integration.

On 02 Feb 2015, at 21:50, Alexander Flatter notifications@github.com wrote:

Opening an issue here to discuss the effects of descendants tracking and the removal thereof. Gerenally, I'd say that descendants tracking is undesirable because it's a class-level interface and hidden from the user. Currently it's used to populate the registries of relations, mappers and commands automagically.

Here's a couple of points to discuss:

Is it currently possible to require ROM objects before calling ROM.setup? Will issues arise when using multiple environments? What's the consequences for the way readers are currently working? AFAIK, a chain of calls to the Reader simply builds a path of said calls, e.g. users.with_posts.active, that is then used to retrieve the appropriate mapper. The alternative to using descendants tracking is to make registry explicit. Note that this won't bother users of rom-rails or similar integration gems as they would ship with support and conventions for location and name of those classes.

require 'my_app/relations/users' setup = ROM.setup(...) setup.register_relation(:users, MyApp::Relations::Users) The advantages are the removal of a fragile technique, independence of load order and setup time, less hassle in the test suite and simplification of ROM core.

/cc @elskwid @solnic @splattael: We already talked quite a bit about this. :)

— Reply to this email directly or view it on GitHub.

solnic commented 9 years ago

@aflatter are you interested in making that change? I have other stuff on my plate that I need to do so this is a nice-to-have for me ;)

aflatter commented 9 years ago

Sure, I am, but can't give an ETA. :-(

solnic commented 9 years ago

Move to 1.0.0 milestone then?

solnic commented 9 years ago

No pressure whatsoever but do you think you could wrap this up this week so that we can include it in 0.6.0?

aflatter commented 9 years ago

I'll try, but I'm behind schedule as I got struck down by a flu. :-(

solnic commented 9 years ago

Sorry to hear that. I'm struggling with an infection myself :/ Seems like we're gonna postpone 0.6.0 release a little bit.

schmurfy commented 9 years ago

That's funny because while looking at the example in the README I was wondering how was everything linked together ;)

I don't like blackmagic much (but I still use it in some scenario) and that's one reason I would love to go away from activerecord, blackmagic is nice as long at the framework does what you want but as soon as you start doing "out of scope" things then the nightmare begins and you have to dig through piles of code (in activerecord case at least) to figure what the hell is happening and why.

Will 0.6.0 be released with this change ?

solnic commented 9 years ago

The setup process is relatively complex but I wouldn't call it blackmagic. Descendants tracking is gone already but we still use inherited hook to register a class. That's an implicit behavior though and potentially can be confusing, otoh it's just part of the setup, this is not something that's happening at run-time after setup.

Can you tell me which part got you confused wondering how on earth that works?

schmurfy commented 9 years ago

When I spoke about blackmagic I was not specically refering to what is currently done in rom but that's more a fear about what happens in other projects ^^

This is not directly related to this issue but how the parts are linked to each other is not really intuitive for me, when looking at the current example in the README a bunch of class are defined but almost none of them are directly referenced afterwards and relations between them are defined inside the class declaration.

For example this:

class UserMapper < ROM::Mapper
  relation :users
  register_as :entity

  model User

  attribute :name
  attribute :age
end

got me scratching my head at first and while I think I understand what it does this is a rather unusual way of doing things, you declare a class referencing other class/entities sometimes by name (:users) and by class (User) and the class itself is never used again directly but instead referenced by another name when doing:

puts rom.relation(:users) { |r| r.by_name("Jane").adults }.as(:entity).to_a.inspect

Since we are not really defining classes I prefer the old notation (will both stay useable ?) with methods and blocks:

setup.relation(:users) do
  def by_name(name)
    where { name == name }
  end

  def adults
    where { age >= 18 }
  end
end

When I look a a this what I am really defining is clear, that's not a class and it does not pretends to be one.

I like the project though, I have been watching it for a long time and it is nice to see that things are starting to take shape :)

solnic commented 9 years ago

Yeah that's why I started with the DSL in the first place. Later on we discovered that it's easier to re-use inheritance mechanisms in ruby vs reinventing them for the DSL purposes.

I believe DSL will stay, maybe it'll be extracted into a separate project (rom-micro? :)) but the class-based approach makes much more sense in bigger projects.

Regarding your concern about defining classes but never having to instantiate them - I can understand the confusion. Fortunately it's easy to explain and we should definitely do a better job here in the documentation.

What you see with the classes and then having all of those instantiated for you is a ROM environment, it's pretty much an object factory that's built into ROM. It simplifies your life as you don't have to deal with object instantiation yourself. I want to take this idea further and extend it with some DI mechanisms too that would work well with ROM core objects.

Keep in mind that it's just a convention, ROM env is just a way to organize those objects, you can instantiate objects yourself if you want and organize them in a different way:

require 'rom/memory'

memory_repo = ROM::Memory::Repository.new
user_dataset = memory_repo.dataset(:users)

class Users < ROM::Relation[:memory]
end

class UserMapper < ROM::Mapper
  model name: 'UserEntity' # ROM supports generating model classes
  attribute :name
end

user_relation = Users.new(user_dataset)
user_mapper = UserMapper.build # using build injects default mapper processor

user_relation.insert(name: 'Jane')

puts user_mapper.call(user_relation).to_a.inspect
# [#<UserEntity:0x007fd9c22eddc8 @name="Jane">]

ROM consists of really simple objects, the most complex part is the setup, as I mentioned, but as you can see those objects are dead simple to instantiate yourself. Also notice that in the example above I didn't use the setup at all.

solnic commented 9 years ago

Oh one more thing - ROM env object also exposes a higher-level interface for accessing relations and commands, it has access to all registries so we can have more convenient interfaces built on top of that.

schmurfy commented 9 years ago

Thanks for the explanations, that was just my first impression after coming back to the project after leaving it aside for some time (last time there only the DSL language, not the classes), I need to play more with it I think to see how things play.

I did not thought about inheritance, does this means you could have a base mapper with specialized ones inheriting from it ? And since the name of the class means nothing, does it means the following will work the same for the mapper ? or does the name still plays a role ?

class PolarBearMapper < ROM::Mapper
  relation :users
  register_as :entity

  model User

  attribute :name
  attribute :age
end
solnic commented 9 years ago

Mapper class name is not used at all right now. We only infer dataset name from relation class name.

Regarding inheritance - if you inherit from another mapper its whole header will be copied and then you can override specific attributes if you want.

muescha commented 9 years ago

here is also blackmagic? this generates a relation for me magic?

class Users < ROM::Relation[:memory]
end

which i can use later as:

relation :users
solnic commented 9 years ago

Nah it is just a shortcut for ROM::Memory::Relation On Fri 6 Mar 2015 at 22:15 muescha notifications@github.com wrote:

here is also blackmagic? this generates a relation for me magic?

class Users < ROM::Relation[:memory]end

— Reply to this email directly or view it on GitHub https://github.com/rom-rb/rom/issues/142#issuecomment-77637075.

muescha commented 9 years ago

i mean it creates the :users for me as magic. in other classes i need register_as - but here not

solnic commented 9 years ago

I hear you. This will soon be replaced by policy-over-configuration so those things will be handled explicitly and you will be able to provide your own policies too (see how it's done in yaks gem). On Fri 6 Mar 2015 at 22:18 muescha notifications@github.com wrote:

i mean it creates the :users for me as magic. in other classes i need register_as - but here not

— Reply to this email directly or view it on GitHub https://github.com/rom-rb/rom/issues/142#issuecomment-77637677.

solnic commented 9 years ago

So, this is finally addressed. We ended up with an optional plugin that enables auto-registration. Disabled by default, to enable you can do ROM.use :auto_registration.