-
Notifications
You must be signed in to change notification settings - Fork 112
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
Add a flag --init to write a default config #81
Conversation
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.
Sub-commands could be useful for things like exporting operations, but that's already supported in the config (and similar things could be added there as well).
I do like how this turned out. I also like explicit context suppression using "-".
// Parse the config that `genqlient --init` generates, to make sure that | ||
// works. |
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.
Nice.
generate/main.go
Outdated
@@ -36,25 +38,32 @@ func readConfigGenerateAndWrite(configFilename string) error { | |||
return nil | |||
} | |||
|
|||
type cliArgs struct { | |||
ConfigFilename string `arg:"positional" placeholder:"FILENAME" default:"genqlient.yaml" help:"path to genqlient configuration (default genqlient.yaml)"` |
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.
Perhaps placeholder:"CONFIG"
?
Steve pointed out (#73) that having genqlient with no arguments silently use a default config file was a bit confusing, and changed it to use `genqlient.yaml` by default (#74). Mark pointed out (#76) that this makes it a bit less convenient when you're starting from scratch; you have to go create a config file. In this commit I add a new init flag that creates you a config file before using it. Originally the suggestion was to use subcommands, e.g. we'd have `genqlient init` and `genqlient generate` and so on. But I couldn't think of anything else we might want subcommands for in the future, and it felt a little silly to make you type `generate` each time. So instead, I made it a flag, which has the nice property that you can do `genqlient --init` and it will generate and then use a config file. (I mean, maybe it will immediately crash because you don't have a schema, but hopefully that's still a useful clue as to what to do next!) The implmentation was fairly trivial. Since we now have a nice way to generate a default config, I removed the default values for most of the options; I've always felt they were probably more confusing than helpful. (And indeed, all the users I know of (Khan/webapp, and the much smaller project Steve was working on, are setting those options explicitly.) I decided this is also a good time to pull in a proper CLI parser (#31); see ADR-504 for more on that choice. This also adds some nice help messages! Fixes #76, #31. Issue: #76 Test plan: ``` go run . go run . --init go run . --init example/genqlient.yaml ``` Reviewers: marksandstrom, steve, adam, miguel
acd5c80
to
9de1cf4
Compare
Summary:
Steve pointed out (#73) that having genqlient with no arguments silently
use a default config file was a bit confusing, and changed it to use
genqlient.yaml
by default (#74). Mark pointed out (#76) that thismakes it a bit less convenient when you're starting from scratch; you
have to go create a config file. In this commit I add a new init flag
that creates you a config file before using it.
Originally the suggestion was to use subcommands, e.g. we'd have
genqlient init
andgenqlient generate
and so on. But I couldn'tthink of anything else we might want subcommands for in the future, and
it felt a little silly to make you type
generate
each time. Soinstead, I made it a flag, which has the nice property that you can do
genqlient --init
and it will generate and then use a config file. (Imean, maybe it will immediately crash because you don't have a schema,
but hopefully that's still a useful clue as to what to do next!) The
implmentation was fairly trivial.
Since we now have a nice way to generate a default config, I removed the
default values for most of the options; I've always felt they were
probably more confusing than helpful. (And indeed, all the users I know
of (Khan/webapp, and the much smaller project Steve was working on, are
setting those options explicitly.) This required a slight change to
the syntax to say "don't use context", which is probably also net clearer.
I decided this is also a good time to pull in a proper CLI parser (#31);
see ADR-504 for more on that choice. This also adds some nice help
messages!
Fixes #76, #31.
Issue: #76
Test plan: