Skip to content
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

Modulize Granite::ORM #49

Closed
c910335 opened this issue Sep 6, 2017 · 6 comments · Fixed by #55
Closed

Modulize Granite::ORM #49

c910335 opened this issue Sep 6, 2017 · 6 comments · Fixed by #55

Comments

@c910335
Copy link
Contributor

c910335 commented Sep 6, 2017

I think it is time to modulize Granite::ORM because src/granite_orm.cr is too complicated to add new features.
The following are my suggestions.

Renaming Granite::ORM to Granite::ORM::Base

If we want to split Granite::ORM into modules, those modules should be inside a module which is very likely to be named as Granite::ORM. Thus, renaming the original class Granite::ORM to Granite::ORM::Base is necessary although it is a breaking change.

The Plan for Modulizing

  • src
    • adapter - unchanged
    • granite_orm
      • associations.cr - things mentioned in this issue add support for 1 to many relationships #30
      • base.cr - require all the other files in src/granite_orm, declare class Granite::ORM::Base, include(extend) the modules and constructors
      • callbacks.cr - things in my previous pr add callbacks #42
      • fields.cr - macro field, timestamps, process_fields (but this is still too large and need partitioning), and method set_attributes
      • querying.cr - current public class methods
      • table.cr - macro adapter, table_name, primary
      • transactions.cr - method save, destroy
      • version.cr - just version
    • granite_orm.cr - require "./granite_orm/base"

There may be some difficulties and this plan still needs improvements, so let's start discussing.

@drujensen
Copy link
Member

@c910335 yes, I agree long overdue. I like your proposal.

We are also looking to move Kemalyst::Validators into Granite::ORM so this modularization would be a welcome change.

Is this something you will be working on?

@c910335
Copy link
Contributor Author

c910335 commented Sep 6, 2017

If there is no progress by now, I am going to deal with this.

@drujensen
Copy link
Member

I"m not aware of anyone working on this. thanks @c910335

@c910335
Copy link
Contributor Author

c910335 commented Sep 8, 2017

Why are there so many class methods defined in the macro process_fields?
I think clear, all, find, find_by, exec, query and scalar should be taken out of it because they didn't access any macro variables.

@drujensen
Copy link
Member

drujensen commented Sep 8, 2017

This is remnants from before having the ability to do a finished macro. https://github.com/amber-crystal/granite-orm/blob/master/src/granite_orm.cr#L15

I believe they used to have macro variables. If we can remove them from the macro now, we should.

@c910335
Copy link
Contributor Author

c910335 commented Sep 10, 2017

Forgot associations.cr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants