Skip to content
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

Provide a pluggable interface for alternative configuration value sources (files, env vars) #44

Closed
arnetheduck opened this issue Oct 10, 2018 · 8 comments

Comments

@arnetheduck
Copy link

A nice feature of command lines is if you can stick the same runtime option values in config files or pick them out from environment variables.

Since cligen builds a mapping of name-to-value, would be interesting if there was support for building up such chains of option-sources from one source of truth about what the valid options are - specially interesting if #30 happens and options can be gathered in objects that are a little bit more complex.

This API would need to give the parser names and types to work with as well as a way to feed back the values read. Further, it would have to support prioritizing, so that something on command line overrides config file.

@c-blake
Copy link
Owner

c-blake commented Oct 10, 2018

So, this is not a bad idea, but there are some thorny sub-issues. A config file has more intrinsic flexibility than a flat name=value parameter list. In particular, it can have hierarchy that parallels hierarchy in some object a la #30 . Contrariwise, an environment variable has even less intrinsic flexibility being a flat string that needs splitting/quoting rules. So, one can maybe just implement the simple case, but it probably wouldn't be long before someone asked for maximum analogousness of an object with a config file (saying they never use command-line args or environment variables) which would then allow more than the other cases could. Just a comment on the idea. I am open to working on a pull request if someone wants to try. To me it seems like almost a parallel "cfParseGen" module would be more likely to be well-received. Part of what makes something like cligen easy to both implement and use is that a command-line "call" and a "proc/func call" are pretty close semantically.

@arnetheduck
Copy link
Author

Yeah, good points. In part, that's why I think a pluggable API might be the way to go, instead of fixating on a particular form and shape of config files - basically, cligen would be split up into layers - one which parses function defs and objects and passes some sort of summary to another part that takes the names and applies them.

The way I've enjoyed a feature like this is for the slightly more complex applications that have several subcommands and several tweaks that can be controlled from the command line. git is one, but another easy-to-grok example is https://github.com/ethereum/go-ethereum/wiki/Command-Line-Options - basically, options are categorized, and in the config file (which is toml), this corresponds to a 2-level hierarchy, something like

[txpool]
option_a = y

which I can override with --txpool.option_a=y etc.

of course, this shouldn't break the fantastic for-dummies mode that cligen has and just works, for the simple case where all this machinery is not needed!

@timotheecour
Copy link
Contributor

#46 is partially related

@c-blake
Copy link
Owner

c-blake commented Nov 9, 2018

So, as I mentioned in #30, you can get your "end CLI user functionality" of merging config file and environment variable sources for command parameters with whatever a user enters on a given command line without any more cligen features. The first argument of dispatchFoo() has always been cmdLine: seq[string]. This is even more flexible now that #47 is closed.

All you really need is to have some mergeParams() helper function to parse an environment var, parse a config file, and merge those two seqs with commandLineParams() with whatever priority. This would work fine for a one-level dispatch and needs no cligen update.

In a dispatchMulti setting, I guess you'd probably want dispatchMulti to pass through the subcommand name to mergeParams(), as well. That way, maybe mergeParams() could take a cmdName parameter and maybe also a subCmdName and look for $CMDNAME and ~/.config/cmdName or ~/.cmdNameRc or whatever with a flat name space if subCmdName=="" but if subCmdName!="" instead look for $CMDNAME_SUBCMDNAME and ~/.config/cmdName with a hierarchical namespace and stuff like [txpool]/option_a ....

So, we could probably just provide some default mergeParams() which does all that and then if the CLI author doesn't like it they can define their own in the scope after import cligen but before dispatch/dispatchMulti. Nim should use their definition, not the default. Would this sort of set up satisfy your needs for pluggability? I believe it's pretty flexible. I'll probably give that a try in the near future.

@c-blake
Copy link
Owner

c-blake commented Nov 10, 2018

Well, I went ahead and added a default mergeParams() impl one can re-define. README.md describes use and test/FullyAutoMulti.nim is a more complete example.

If you (or anyone else) wants to write something that uses the Nim stdlib's parsecfg and maybe some env.var parsing fancier than split I'd be fine distributing that inside cligen.nim. I would say just two procs would do it and then let people write their own tiny mergeParams() = config & env & cmdLine. All those not-in-user's-face sources could create more of a need for some kind of lastWriterWins parameter filter, but I'd say that's a very separate issue (as even script wrappers could also create such a need). So, closing this issue.

Personally, I am my own primary user for all the commands I write with cligen. So, my defaults for params (other than command line changes) are basically always what I already want. Hence, I don't have much/any need for env or config sources to change them. ;-) I would surely bend to popular demand if people wanted the default mergeParams() to change.

@arnetheduck
Copy link
Author

cool, thanks! I'll check that out for sure and get back when I have some feedback :)

@c-blake
Copy link
Owner

c-blake commented Nov 12, 2018

Cool. About the only real question in my mind was whether mergeParams should take a names: seq[string] instead of an already assembled qualifiedName: string.

There are pros & cons...Making users doing simple commands have to use [0] vs users doing complex commands having to maybe split('_') and worrying about delimiting. I tilted toward making the easiest case dirt simple. That also gives the two parameters to mergeParams different types which catches mixing up their meaning. The seq[string] approach also generalizes if a dispatchMultiMulti ever happens as well as side-stepping delimiting issues, though. So, I'm leaning toward making that change to a seq before punching a release button. It's like a 5 minute change, but I'll let you feed back before doing it.

BTW, I didn't say before, but the "one source of truth", at least in cligen, should always be the proc being dispatched to. That's the organizing principle - no need for some separate specification source/language/complexity when you already have that. But I do like this any old source of seq[string] besides os.commandLineParams(). So, thanks for prodding me on this.

@c-blake
Copy link
Owner

c-blake commented Nov 16, 2018

Well, I gave it almost a week, but decided I should not leave a temporary interface in the git repo for too long. So, I went ahead and made that cmdName: string -> cmdNames: seq[string] change. I'm pretty sure it's the right way to go, if for no other reason than sidestepping the question of delimiting cmdName via "_" or "/" or something more general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants