search

spider-gazelle / rethinkdb-orm

RethinkDB ORM for Crystal lang
MIT License
24 stars 0 forks source link

Make RethinkORM::Base a module #2

Open watzon opened 4 years ago

watzon commented 4 years ago

I'm going to echo the same comment I made with Granite. It would be really nice if RethinkORM::Base were a module rather than an abstract class. The main two reasons I can think of as to why this would be good are:

  1. You want this to be an optional dependency, so in a class you want to override some class definitions and include RethinkORM::Base (my case)
  2. Your model class needs to extend another type. Having to extend this will keep you from doing that.

This would obviously be a breaking change, but maybe it doesn't have to be. If the logic in RethinkORM::Base were moved into a module with a different name, and then that module were included into RethinkORM::Base we could probably have our cake and eat it too.

Thoughts?

caspiano commented 4 years ago

Hey, thanks for the suggestion! I agree. This library could be beneficially refactored into a module to allow for looser coupling however there's quite a bit of dependency on active-model.

I'll have a think about this over the next few days and see if I can come up with anything better.

watzon commented 4 years ago

Awesome! It would be a lifesaver for me if it would be possible. Right now there aren't many ORMs that use a module. Most use the abstract class approach.

caspiano commented 4 years ago

Unfortunately, I haven't had time to work on this 😕 My current priority with this project is to add connection pooling, as well as a few other touch-ups. But the decoupling is definitely on the agenda.

Feel free to open up a PR if you're up for it 🙂