Repository navigation
Add generator option for SQL migrations - #470
Conversation
katafrakt
left a comment
katafrakt
left a comment
There was a problem hiding this comment.
A couple of nitpicks from me
| class Migration < Command | ||
| argument :name, required: true, desc: "Migration name" | ||
| option :gateway, desc: "Generate migration for gateway" | ||
| option :sql, type: :boolean, default: false, desc: "Generate SQL migration" |
There was a problem hiding this comment.
I think I would be confused by this description if I encountered it outside of this PR context. Perhaps "Generate a plain SQL migration" would be better?
There was a problem hiding this comment.
If both filetypes are to be considered a "SQL migration" I think that is more confusing. Migrations come from ROM::SQL, so their applicability to only SQL adapters is assumed.
The distinction I wish to make here is one of filetype: Ruby or SQL. So, how about: "Generate SQL (default Ruby)". This is the migration generator so I think we can drop that as redundant, and this I think would indicate more clearly that it's regarding file contents.
There was a problem hiding this comment.
"Generate SQL (default Ruby)" is a second option I considered. After reading your explanation I think it's better than what I proposed.
There was a problem hiding this comment.
Maybe the issue is option :sql, type: :boolean? Maybe the option could be type and the arguments could be either rb or sql, with rb the default? That seems to serve the purpose and is more explicit.
| class Migration < Command | ||
| argument :name, required: true, desc: "Migration name" | ||
| option :gateway, desc: "Generate migration for gateway" | ||
| option :sql, type: :boolean, default: false, desc: "Generate SQL migration" |
There was a problem hiding this comment.
Thinking a bit ahead, do you think we would like to have an app-level setting which would set the migration type to plain SQL by default? In such case this would not be a good option name - it should rather be "format" or "type" instead, I think.
There was a problem hiding this comment.
I'm not sure about whether an option is called for, but assuming not I'd rather not have to type --format=sql by hand. That would make more sense if it were a possibility that more formats would follow, but I think it is highly unlikely we will ever want more than these two. We could potentially add a second --ruby option that is mutually exclusive. Basically I wanted the shortest option that was still clear enough what was happening.
There was a problem hiding this comment.
If we'd have a setting, you would not need to type anything at all - just hanami generate migration will generate an SQL migration every time. So I guess the question is which kind of DX we need: be explicit and have to supply a short option every time or just have a default for the project.
That being said, I think having mutually exclusive --sql and --ruby is a good path forward, so we might leave just --sql here then.
There was a problem hiding this comment.
I don't think of this as all-or-nothing. The SQL format is intended for complicated migrations that are hard to express in Ruby, but I think it's quite likely you will want a mixture of both.
4722bdb to
2c4b457
Compare
Relates to hanakai-rb/rom-sql#440