-
Notifications
You must be signed in to change notification settings - Fork 88
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
DSL for migrations #413
DSL for migrations #413
Conversation
Given the result of #182, is this something we actually want to include? If so, I think I would rather remove, or at least deprecate, the current internal migrator so we don't end up with just another way to do the same thing, as this would be the third way of doing migrations. |
@Blacksmoke16 Over the years, I have collected feedback on why developers prefer other ORM's over granite and the lack of a DSL for DDL rises to the top. This is not a full featured DSL yet. There are several options that still need to be built into it. Also, there are some missing types yet to be added. Currently this is a self contained class with little changes to existing code. I could see merging or having Let me know your thoughts. I can close this if you think its not worth the effort. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drujensen I think this is a great start, and a good set of footsteps to follow in. Granite's dependence on Micrate has frustrated me many times and my experience with Rails has been that an abstracted DDL is helpful because the "Right Choices" can be made once in the abstraction layer and down the line nobody has to think about it.
Reading the current implementation leaves me with some thoughts:
- Support for multiple adapters is minimal. There's a steady stream of people asking for Postgres specific data types and I'd be surprised if that didn't happen here too. However the escape mechanism of "use the execute with raw sql" is helpful in the short term. I'm not sure what a good solution to that is -- the only thing that comes to mind is loading a different DSL class for a different adapter. With inheritance that might not be too bad.
- I don't see any support given for default values, and I think I'd consider this important enough to have before rolling it out.
@drujensen Personally I'm fine with raw SQL migrations but if this is a commonly requested feature I don't have a problem with the addition. That is, assuming you/someone will be maintaining it. |
@Blacksmoke16 @robacarp thanks for spending the time looking at this. @robacarp I agree about the Regarding the specific data types per db, I agree with this as well. I will look into refactoring out the |
def up | ||
create_table :posts do |t| | ||
t.primary :id | ||
t.serial :serial1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the dsl, how can we specify other column parameters, such as size, not null, index etc?
In the same format it can provide a t.constraint
, t.unique
, t.belongs_to
etc
Thinking about this more, this should be included in the micrate project instead of here. Closing this PR. |
This adds a new DSL for migrations. It uses the same syntax as Rails migrations.
To execute: