Fix global cli argument handling #1304
Conversation
Here we could not use the global argument thing because --migration_dir is no global argument, instead we have to look in each subcommand for the corresponding argument
* 2 Testcases for --migration_dir * Readd the tests from diesel-rs#1201 for --database_url
| matches | ||
| .value_of("MIGRATION_DIRECTORY") | ||
| .map(PathBuf::from) | ||
| .or_else(|| { |
There was a problem hiding this comment.
@weiznich - I'm still becoming familiar with rust idioms and clap! 😅. From what I understand, Pathbuf::from indicates that you expect the MIGRATION_DIRECTORY value will always convert without fail. Why do we need to chain .or_else and then match on the subcommand (name?, not sure which value of the tuple 1 represents)?
There was a problem hiding this comment.
From what I understand,
Pathbuf::fromindicates that you expect theMIGRATION_DIRECTORYvalue will always convert without fail.
As far as I understood the stdlib docs/code this conversion is guaranteed to succeed. If the user passes a nonsense path a PathBuf pointing to a non existing (the nonsense path) location is generated and the following operations executed by diesel will end with a error message indication a invalid/non existing path.
Why do we need to chain
.or_elseand then match on the subcommand (name?, not sure which value of the tuple 1 represents)?
The or_else is needed to workaround some architectural decisions/limitations in clap. A clap app consists of several subcommands (migration, setup, print-schema , … for diesel_cli). Each subcommand could also consists of several subcommands (list, run, … for diesel migration). Arguments are defined in a subcommand and are valid for the defined subcommand and all child subcommands. ArgMatches contains all arguments passed to a certain subcommand. Arguments are stored in that ArgMatches to which they are passed, not in which they are defined. So if someone is calling diesel migration --migration_dir /some/dir list the migration_dir argument is stored in the ArgMatches of migration. It is also possible to call diesel migration list --migration_dir /some/dir where migration_dir is stored in the ArgMatches of list. (There is a global option that allows a argument globally, but that does not solve this problem here). Therefore we need to traverse the results for each subcommand. This is done by recursively calling this method on each subcommand. (See the clap docs for why the we access .1 on the tuple)
There was a problem hiding this comment.
Awesome!!! 😄 Thanks so much! I'll finish review ASAP this week (unless someone else beats me to it).
|
@killercup @Eijebong - I'd like to merge this. Good to go? |
|
LGTM! Go ahead, @notryanb! |
| @@ -16,7 +16,7 @@ path = "src/main.rs" | |||
|
|
|||
| [dependencies] | |||
| chrono = "0.4" | |||
| clap = "2.20.3" | |||
| clap = ">=2.27.0" | |||
There was a problem hiding this comment.
Diesel CLI 0.99 is going to break when a new version of clap is released...
Fixes #1301
This is a improved/newer version of #1201