Default create extension to sql #53
Conversation
Pull Request Test Coverage Report for Build 89
💛 - Coveralls |
|
Thanks for opening a PR but using a default extension isn't the appropriate fix as there are other DBs A better approach is to present better error messages. |
|
My reasoning is that a "default" value is the one that works for most people. The empty string is clearly not a good default because it doesn't work for anyone and is just a bug. I chose .sql because I'm guessing it is the most common option for your users of your project. This seems reasonable since all the documentation uses the .sql extension in examples. Obviously, if .sql worked for everyone, changing extension wouldn't be a CLI flag at all and the -ext flag should just be removed. Your approach would make the -ext mandatory for the create call. IMHO, that would reduce the usability of the tool. Feel free to close if you disagree or have a different solution, it's your party. |
|
It looks like we have the following solutions for this problem:
As suggested by @smw1218, I like the 2nd solution the most since it works the best with existing commands and docs and most migrations should have an extension anyways. @smw1218 do you mind updating this PR to make the |
|
Flags in Go cannot be made mandatory, so I'll leave the default as the empty string and error if the extension is not specified. |
|
I still think it's a mistake to make a parameter mandatory when most users just want .sql |
|
I disagree. It's not clear that most users are using SQL. Although most DBs supported by Making the Evaluation of potential solutions
Of the potential solutions listed above, the only one that best maintains backwards compatibility and improves usability is 4. However, I've currently implemented 2 instead of 4 since it keeps constraints instead of relaxing them, but I could go with either solution for the next release. |
|
Solution 2 was implemented in v3.3.0 |
The default extension is left blank even though the parameter is optional. This produces a file that has no extension at all which then doesn't match the regexp in source/parse.go (a broken migration file name). Defaulting to .sql fixes this and matches your documentation.