Walk parent directories to find config file #141
Conversation
johnmaguire
requested review from
benjaminjkraft,
csilvers,
dnerdy and
StevenACoffman
as code owners
|
@johnmaguire Can you add test coverage for findCfg and findCfgInDir? The addition of |
|
@StevenACoffman I removed the dependency on |
There was a problem hiding this comment.
I am wondering about documentation -- where is the right place to document this behavior? Maybe not the help text, but in the README maybe? I do think you want to add an entry to the changelog for this.
If you don't mind waiting a few days, it would be nice to wait for @benjaminjkraft to weigh in on the semantics -- whether allowing the directory search is desirable. I don't see a problem with it myself, but I do know that a 'chdir' in code is a red flag for some, so it might be good to get his opinion.
There was a problem hiding this comment.
Thanks for the contribution! This seems like a very reasonable feature to me.
I mention inline one place to document it. It would also be nice to add a mention of where genqlient looks for the config file at the top of the genqlient.yaml documentation; just to say that you can pass it a path or by default it looks for such-and-such filenames in ancestor directories.
BTW, I'm not necessarily opposed to chdir, as long as it doesn't cause surprising behavior elsewhere, but I think in theory you shouldn't need it! In which case best not.
generate/main.go
Outdated
| @@ -42,7 +51,7 @@ func readConfigGenerateAndWrite(configFilename string) error { | |||
| } | |||
|
|
|||
| type cliArgs struct { | |||
| ConfigFilename string `arg:"positional" placeholder:"CONFIG" default:"genqlient.yaml" help:"path to genqlient configuration (default genqlient.yaml)"` | |||
| ConfigFilename string `arg:"positional" placeholder:"CONFIG" default:"" help:"path to genqlient configuration (default genqlient.yaml)"` | |||
There was a problem hiding this comment.
You'll need to change initConfig, below, to also handle ""! I think you can just have it default to genqlient.yaml (in the current directory).
Also, if you can briefly document the new behavior here, e.g. "(default: search ancestors of the current directory for genqlient.yaml)".
There was a problem hiding this comment.
Updated the documentation, let me know if it's OK or feel free to fix the wording as preferred.
generate/config.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| err = os.Chdir(filepath.Dir(cfgFile)) |
There was a problem hiding this comment.
There shouldn't be a need to chdir, I think! Did you see problems without it? As long as cfgFile passed to ReadAndValidateConfig is an absolute or current-directory-relative path, everything should be fine.
There was a problem hiding this comment.
No, I didn't. I wasn't sure if the program relied on the current directory for anything besides the config file path, so I figured it was safer to add it.
It seems to be working for my limited use case, so I've pushed a commit to remove it.
|
Thanks for merging this! |
Code is mostly borrowed from gqlgen.
The idea here is that I want to be able to store
genqlient.yamlat the top-level, but my client code lives down ingraph/client/. I put the//go:generateline ingraph/client/client.go.