Several significant changes #3

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
3 participants
@elight

elight commented Mar 15, 2013

  • More automation; reduces the amount of work to define a Domain Model class
  • Removed ORM abstraction layer; edr is now dedicated specifically to AR
  • Minor refactorings

More API changes and refactorings to come.

I'm uncertain whether this deviates enough from the original version to warrant a full blown fork/separate gem. I'd say it depends upon how far it deviates from your goals for the gem.

-module Edr
- module AR
- module Repository
- include ::Edr::Repository

This comment has been minimized.

Show comment Hide comment
@vsavkin

vsavkin Mar 19, 2013

Contributor

We use EDR with different ORMs (such as Sequel), so removing the ORM layer doesn't really work for us. Why is it necessary?

@vsavkin

vsavkin Mar 19, 2013

Contributor

We use EDR with different ORMs (such as Sequel), so removing the ORM layer doesn't really work for us. Why is it necessary?

end
+ def self.map_models_to_mappers

This comment has been minimized.

Show comment Hide comment
@vsavkin

vsavkin Mar 19, 2013

Contributor

Not sure if using naming conventions will work in all the cases. Consider the case when the same data object is used by multiple models, or when some legacy class is used as a data class.

@vsavkin

vsavkin Mar 19, 2013

Contributor

Not sure if using naming conventions will work in all the cases. Consider the case when the same data object is used by multiple models, or when some legacy class is used as a data class.

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 19, 2013

That's the thing: that's not the 80% case. The average developer won't encounter that situation.

On Tue, Mar 19, 2013 at 1:18 PM, Victor Savkin notifications@github.com
wrote:

 end
  • def self.map_models_to_mappers
    Not sure if using naming conventions will work in all the cases. Consider the case when the same data object is used by multiple models, or when some legacy class is used as a data class.

Reply to this email directly or view it on GitHub:
https://github.com/nulogy/edr/pull/3/files#r3435984

@elight

elight Mar 19, 2013

That's the thing: that's not the 80% case. The average developer won't encounter that situation.

On Tue, Mar 19, 2013 at 1:18 PM, Victor Savkin notifications@github.com
wrote:

 end
  • def self.map_models_to_mappers
    Not sure if using naming conventions will work in all the cases. Consider the case when the same data object is used by multiple models, or when some legacy class is used as a data class.

Reply to this email directly or view it on GitHub:
https://github.com/nulogy/edr/pull/3/files#r3435984

def add_item attrs
repository.create_item self, attrs
end
end
class Item
- include Edr::Model
-
- fields :id, :name, :amount, :order_id

This comment has been minimized.

Show comment Hide comment
@vsavkin

vsavkin Mar 19, 2013

Contributor

I think a model should declare its fields. Pulling them automagically from the corresponding data object is very implicit.

@vsavkin

vsavkin Mar 19, 2013

Contributor

I think a model should declare its fields. Pulling them automagically from the corresponding data object is very implicit.

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 19, 2013

Yes it is. It's also the Rails Way.

It's ok. I'll fork the project, rename it, credit you and the other precious devs, and carry on in a different direction.

As I wrote in the PR, I suspected that my approach deviates from where you were going. But my intended audience, I believe, is also different.

On Tue, Mar 19, 2013 at 1:22 PM, Victor Savkin notifications@github.com
wrote:

def add_item attrs
repository.create_item self, attrs
end
end

class Item

- include Edr::Model

  • fields :id, :name, :amount, :order_id
    I think a model should declare its fields. Pulling them automagically from the corresponding data object is very implicit.

Reply to this email directly or view it on GitHub:
https://github.com/nulogy/edr/pull/3/files#r3436098

@elight

elight Mar 19, 2013

Yes it is. It's also the Rails Way.

It's ok. I'll fork the project, rename it, credit you and the other precious devs, and carry on in a different direction.

As I wrote in the PR, I suspected that my approach deviates from where you were going. But my intended audience, I believe, is also different.

On Tue, Mar 19, 2013 at 1:22 PM, Victor Savkin notifications@github.com
wrote:

def add_item attrs
repository.create_item self, attrs
end
end

class Item

- include Edr::Model

  • fields :id, :name, :amount, :order_id
    I think a model should declare its fields. Pulling them automagically from the corresponding data object is very implicit.

Reply to this email directly or view it on GitHub:
https://github.com/nulogy/edr/pull/3/files#r3436098

This comment has been minimized.

Show comment Hide comment
@vsavkin

vsavkin Mar 20, 2013

Contributor

Yup, I think our approaches are rather different and forking the project is the way to go.

@vsavkin

vsavkin Mar 20, 2013

Contributor

Yup, I think our approaches are rather different and forking the project is the way to go.

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 20, 2013

Rock on. Pleasure having a rational OSS discussion with someone. 👍

On Tue, Mar 19, 2013 at 8:48 PM, Victor Savkin notifications@github.com
wrote:

def add_item attrs
repository.create_item self, attrs
end
end

class Item

- include Edr::Model

  • fields :id, :name, :amount, :order_id
    Yup, I think our approaches are rather different and forking the project is the way to go.

Reply to this email directly or view it on GitHub:
https://github.com/nulogy/edr/pull/3/files#r3443945

@elight

elight Mar 20, 2013

Rock on. Pleasure having a rational OSS discussion with someone. 👍

On Tue, Mar 19, 2013 at 8:48 PM, Victor Savkin notifications@github.com
wrote:

def add_item attrs
repository.create_item self, attrs
end
end

class Item

- include Edr::Model

  • fields :id, :name, :amount, :order_id
    Yup, I think our approaches are rather different and forking the project is the way to go.

Reply to this email directly or view it on GitHub:
https://github.com/nulogy/edr/pull/3/files#r3443945

@ruprict

This comment has been minimized.

Show comment Hide comment
@ruprict

ruprict Mar 21, 2013

So, how do you propose to do this in Rails? You can't just call map_model_to_mappers in an initializer without manually loading all the models first. If you take out the eval of the block, I have to do something like:

edr = Edr::Registry.define
edr.map Model, ModelData
... keep going for each class ...

in development which is kinda gross. I am fully prepared to not understand if there is a better way...

So, how do you propose to do this in Rails? You can't just call map_model_to_mappers in an initializer without manually loading all the models first. If you take out the eval of the block, I have to do something like:

edr = Edr::Registry.define
edr.map Model, ModelData
... keep going for each class ...

in development which is kinda gross. I am fully prepared to not understand if there is a better way...

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 21, 2013

Owner
Owner

elight replied Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 21, 2013

Owner
Owner

elight replied Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@ruprict

ruprict Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 21, 2013

Owner
Owner

elight replied Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@ruprict

ruprict Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 21, 2013

Owner
Owner

elight replied Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 21, 2013

Owner
Owner

elight replied Mar 21, 2013

This comment has been minimized.

Show comment Hide comment
@elight

elight Mar 21, 2013

Owner
Owner

elight replied Mar 21, 2013

@vsavkin vsavkin closed this Apr 10, 2013

@ruprict

This comment has been minimized.

Show comment Hide comment
@ruprict

ruprict May 6, 2013

FWIW, ObjectSpace is disabled in JRuby by default, so this won't work unless you enable it, which is (from what I can tell) a pretty big performance hit. https://github.com/jruby/jruby/wiki/PerformanceTuning#dont-enable-objectspace

FWIW, ObjectSpace is disabled in JRuby by default, so this won't work unless you enable it, which is (from what I can tell) a pretty big performance hit. https://github.com/jruby/jruby/wiki/PerformanceTuning#dont-enable-objectspace

This comment has been minimized.

Show comment Hide comment
@elight

elight May 25, 2013

Owner

Makes sense. But then you still need a way to locate arbitrary subclasses of AR::Base conforming to the naming conventions. JRuby may have it's own way to do this?

Owner

elight replied May 25, 2013

Makes sense. But then you still need a way to locate arbitrary subclasses of AR::Base conforming to the naming conventions. JRuby may have it's own way to do this?

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