-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow adding "external argv"s to be parsed alongside/before command line #748
Comments
This relates to #251 I'm not against this at all, but I don't have the bandwidth to implement it just yet. I'll keep it on the issue tracker. I'd say supporting YAML and TOML would be fine. The part I'd still need fleshed out is whether a user can use both the command line and this config file as well? Does one override the other? What about arguments that accept multiple values, and are passed both in the command line and config files, do they get overridden or appended to? |
I think that a enum MergeType {
Override, // replace values with those from config file
Defaults, // use values from config file only where not given on command line
Conflict. // complain if any value is provided in both places
}
...
matches.merge_from_config(config, MergeType::Override); For arguments that accept multiple values, I think that until someone has a specific use case for merging values from both places, it's probably best to only allow overriding or ignoring. |
This is actually something I'm going to have to deal with in a project soon. (In Python, I tend to dodge the issue by hard-coding my defaults at the top of the only (or top-level) While I'm not familiar with clap internals, I don't see why it would be necessary to over-complicate things. Let's start by separating what clap needs out from a more generic "config file for my program" solution.
With all of that said, what I'd suggest is that it's not really the config file parsing you need to focus on, but providing a clearly documented API that allows something like That would also allow clap defaults to be embedded into larger JSON/TOML/etc. configuration files without complicating clap. Heck, with a little thought, the "from command-line arguments" functions you already have could just be parsers on the same tier as JSON/TOML/etc. which just feed into the API I'm proposing. |
I'm not sure I really buy this. If I have a flag --foo that takes a value N, then it seems reasonable for me to want to assign a default value to it in a config file, but retain the right to want to override it on the actual command line. Depending on how the --foo flag was defined, this might not be allowed in the form of |
You misunderstand. I made only two claims and neither was that
What I'm arguing is that:
Hence, orthogonal. First, let's discuss the "never produces worthwhile behaviour" point, starting with a few examples:
This derives from two issues:
Or, let's come at it from the other direction and address
The former case is counter-productive and the latter sounds like trying to kitchen-sink some kind of user permissions system into clap when that's a job for code which runs after clap is done. Finally, The problem is that, by their very nature, command-line arguments are also the most suitable for one-off overrides, which means that there is no justifiable reason to use Therefore, it's much better to just make defined-precedence overriding an inherent, unavoidable feature of how conflicts between multiple sources are resolved. It's the de facto standard way to handle things and it's the only solution which addresses all of the concerns I brought up. ...it's also easy for people to conceptualize. For example, in Python (since it's pseudocode-like): matches = dict()
matches.update(parse_global_config())
matches.update(parse_local_config())
matches.update(parse_command_line_args()) Therefore, the concept of We may indeed need to discuss the
The only potential kink I see is to at least warn users that step 3 might reset The beauty of this approach is that it's made of very small, very extensible pieces and you can get a lot of utility early on, then amend it later. Here are some example steps:
What I'm envisioning is that the
|
@ssokolow Frankly, I'm having a really hard time following any of what you're saying. In particular, I find it hard to tie what you're saying to a concrete user experience. I'm probably missing some important context. In any case, I discussed some of the issues I personally see with config files: BurntSushi/ripgrep#196 (comment) |
Ugh. That's the worst kind of response because it's a perfectly fair one, yet it's the hardest to formulate a meaningful answer to. Give me an hour or two to do "morning routine" stuff and I'll see what I can do to come at it from a different angle and simplify it so that, at the very least, maybe we can figure out where the disconnect is. |
OK. From a user-experience standpoint, the most intuitive solution is to have later arguments always override earlier ones and to treat the fallback chain from command-line to user defaults to global defaults the same way. That gets you 99% of the way to supporting all behaviours I've seen in the wild. For example:
The end result clap should return is (ie. They shouldn't have to type What I'm arguing is that it's very simple to accomplish this as a collection of small, easily-composed pieces:
Getting paths to things like XDG configuration directories is external to clap, because we don't want to preclude parsing config files stored elsewhere or tie clap to a specific implementation. |
I've read all the comments and have some thoughts on the matter but I'm on mobile right now so I'll try to write up my thoughts early to tomorrow. It boils down to I'd like command line to override config files or env vars, but those two extra sources to be added in a first come first serve manner. I'd also like to limit claps responsibility to parsing arguments from a source. Not determining source order or source validity. I'll type up my full thoughts soon. |
I fully agree. Hence my idea limiting clap to:
It then becomes easy for the clap-using application or 3rd-party crates to implement things like:
As long as you provide examples of how to accomplish all of this with minimal boilerplate using 3rd-party crates, you can indefinitely defer the question of whether anything else belongs in clap itself.
I look forward to it. |
Ok so it took me a little longer to get to this than I'd planned, but here's where I stand on the issue. First, I'm not terribly interested in clap handling the file I/O part of this, i.e. the reading files, handling permissions, errors that come from this, precedence, etc. I intend for this to all happen in the consumer code. The consumer would then pass in the deserialized config representation, for now I'm only imagining TOML/YAML, but others could be added. The goal is ultimately to get clap the info it needs to do it's job, i.e. a normalized structure of an argv. In clap's case simply an ordered vector of strings (meaning anything from OsStr, String, etc.). I'm OK with giving clap either a The details would probably be something like defining a trait let some_env_var = env::var("SOME_ENV_VAR").ok();
let global_cfg = load_toml("/etc/myconfig.toml");
let user_cfg = load_toml("~/myconfig.toml");
let m = App::new("test")
.external_argv(some_env_var)
.external_argv(global_cfg)
.external_argv(user_cfg)
.get_matches(); If the consumer wants This makes parsing very simple because internally clap just parses them in reverse order, and if it reaches an arg that arleady exists in the matches, it just skips it. There are two outstanding issues that this would present though. One is if you have an arg that accepts multiple values, and has a value in some external argv, should a later value in either another external argv or via the explicit command line add to these values, or entirely override? The proposed system above entirely overrides, which I'm more of a fan of. If a consumer wants to provide a global default and allow users to add to those values, I'd be easiest to simply tell the user to include that default value in their own "overrides" or just re-add that value back after clap is done with it's parsing. The second issue where this MergeType came into play. Since clap allows two types of conflicts, POSIX style overrides and hard conflicts, a MergeType would allow consumers to effectively convert from hard conflicts to POSIX style overrides only in the case of external argv conflicts. Basically it gives the choice of whether they want hard conflicts or overrides. This situation, in my estimation, only happens in user defined configs. I.e. a user wants to specify a default that conflicts with other options, but yet may want to override that behaviour at some point. I can't imagine why I consumer would put a conflicting argument into a default config and actually want a hard conflict. Think of unix style aliases, I'm of the thought that all conflicts arising from external argvs should be treated as overridable, and it should just be documented well. I can't think of a concrete example where I'd actually want a hard conflict because I explicitly set something via the commandline. This may sound like I'm in favor of a MergeType, but actually the more I think about it, the less I am. As I stated earlier, I'd prefer to treat all things as overridable, and disallow adding values at the commandline to values defined in the configs. The only thing left to determine is how to represent free/positional arguments in these configs. Another 🚲 🏠 for sure, but options, and flags are simple. I'd suggest simply using a single "args" key and assigning the values in sequence such as Thoughts? |
I wasn't clear about the positional args part, we could equally as easy use the key to individualize them, but I kind of like that they're forced to be in order with only a single key to keep from any confusion by accidentally putting them in the wrong order |
We basically agree on the design aside from whether the merging should be declarative or procedural. Your declarative approach is definitely nicer to look at, but it's puts more onus on clap to support edge-case features (or constrain users by refusing to), as I'll answer in reply to one of your outstanding issues...
That's part of the reason I wanted the merge to be a later step. It allows these two cases to be implemented in the consumer:
It also has the benefit that there's less uncertainty about whether clap will allow users to reuse the same let m = App::new("test")
let matches1 = m
.external_argv(some_env_var1)
.get_matches_from(args1);
let matches2 = m
.external_argv(some_env_var2)
.external_argv(user_cfg2)
.get_matches_from(args2);
let matches3 = m
.external_argv(some_env_var3)
.external_argv(global_cfg3)
.external_argv(user_cfg3)
.get_matches_from(&[]);
At the very least, you'll want it to be With that said, this is definitely a tricky thing to address because:
(Wrapper scripts and shell functions are the standard, accepted way to augment or meddle with positional arguments. As someone who cares about user experience, I might go as far as slipping my own filter in between I'd just treat positional arguments in config files as a validation failure and wait to see if anyone complains, since it can be relaxed without breaking anything. (I've never seen a config file or environment variable that allows specifying positional arguments in 10 years of using DOS/Windows command-line applications and another 16 of using Linux ones. It's always been wrapper scripts or shell features like ...or, in the case of DOS and Windows, wrapper scripts as REM DOS
move %1 %2 %3 %4 %5 %6 %7 %8 %9 %HOME%\DONE\
REM Windows
move %* %HOME%\Done\ Finally, since you didn't mention them either way, here are a couple of other points:
(Without access to the schema, tell me whether |
IMO, that's a little bit of a niche use-case. I'm not saying it's uncommon, but I think it's FAR more common to simply provide these "defaults" in a predetermined location. Unless you're using aliases, typing Due to how the internals of clap work, you can't just have a Also parsing the arguments is very fast, it's building the
There shouldn't be any issue with re-using the same
That's the plan. As far as using a subtree, that should work, depending on the deserialization framework. I.e. this
The
Good point. @nabijaczleweli has a branch with a parser which does all the whitespace/quotation handling that we could use/adapt. Although, the more I think about it, the more I'd rather do one of the following
Edit: updated Option 2 about the positional args |
I don't know whether anyone knows about this here: https://docs.rs/preferences/1.1.0/preferences/ But it seems you could just use this as an optional dependency instead of a separate implementation within clap to achieve the same thing. The clap would still be clap instead of swiss army knife of being a configuration file as well as command line parser. |
I reckon ConfigArgParse package for Python is a terrific reference for a good useful and featureful implementation. |
While looking into solutions to this issue I came across the Tthe only fault being the amount of boilerplate needed for defaults - serdes expects to use the Default trait, while StructOpt wants those as strings in field attributes. The code sample in structopt#150 shows how to have a single authority for those defaults in |
Actually, |
The limitation to both is the type needs to implement |
I must have mistunderstood how StructOpt works then, because to me it seemed as if the no-param Edit: I do not want to be argumentative, just stating my observations - a seven value config has ballooned to over fifty lines of code using the method I mentioned. @Kixunil: I indeed overlooked serde's option of using functions, but I don't think this helps here, especially with the difference in types. |
Yes, I thought you had meant
We support any expression. In general, we need to loosen up our attributes to allow any expression so there isn't a question of what you can or can't do (#3173) |
@epage seems like clap V3 will be indeed a big change in a good direction! I'd still love to see file handling too, but understand that you're reluctant to include it. I've given it some thought myself and it indeed is a tricky thing. |
@jaskij note there is also a discussion on layered config at #2763, exploring different options. I also include the option I've landed on for now |
Came here looking for the ConfigArgParser functionality as well. I think it's fine to leave the config parsing as an exercise for an external package... The problem I'm having is that if I really use clap and it's handling the defaults and things; then there's no interfaces available for me to complete the merge with commandline overwriting config if specified. Ideally, I'd like to use --name value if it's there, and fall back on config name if it's there and finally try clap --name default if there wasn't anything else to use. The problem is I don't see any obvious way to test the matches to see if the user specified the value or if clap fell back on the specified default so there's no way to complete such a merge. Another strategy I've used in the past is to just read the config from the usual places (and give up on the idea of a --configfile flag) and use anything from the config files as the defaults for the switches and things. But I don't think you can do that at all with either the builder (using arg!()) or with especially with the derived macro interface things. I'm pretty new to rust, but this seems like a bit of a showstopper on using clap for my current little project... which really bums me out because clap is so neat and there seem to be very few alternatives (perhaps none). |
btw if you want to read up on more thoughts on layering clap with config, check out #2763 |
On Tue, Aug 30, 2022 at 3:58 PM Ed Page ***@***.***> wrote:
The problem is I don't see any obvious way to test the matches to see if
the user specified the value or if clap fell back on the specified default
so there's no way to complete such a merge.
matches.value_source("arg")
<https://docs.rs/clap/latest/clap/parser/struct.ArgMatches.html#method.value_source>
will let you know what set the value.
@epage, value_source looks pretty slick. Thanks for mentioning it.
Looking at #2763 it is hard for me to tell if the (eventual) plan for clap
is to incorporate some sort of ConfigArgParse functionality. Is that
something that clap will eventually provide, or will it always be left up
to the application to parse options from a config file?
Thanks for the help and answers... and code!
…-mMessage ID: ***@***.***>
|
The recommended route for layering configs is still TBD. There are multiple strategies people can use now and some improvements that we are making for v4 that will unblock other strategies. |
Oh, fantastic. Thanks very much for the tips. I feel like there's ways out of the trap now and I can stick with clap (which I like very much). Game on. I'm daydreaming about coming up with an external crate that solves the problems I want regards to ConfigArgParse behavior. I like the idea that rust packages can be "finished" and to avoid feature creep, this new functionality should perhaps be added in another package. I'm just not the right guy for the job yet as I'm still very new to rust. I'm encouraged that the stubs/api-thingies exist to let this happen in another crate. [edit] it also looks like others have already done the things I was thinking about but I didn't know what to search for. (e.g., clap_conf looks promising). Hrm, that seems tied to ^2.33.0 ... oh well, some day. |
@epage, would you be willing to mention the different "multiple strategies" and also what improvements to v4 that have been made to unblock other strategies, and what those "other strategies" would be? Thanks! |
#2763 explores the different options. clap v4 added |
Maintainer's notes
I wrote up a sketch of how this might look here:
https://github.com/casey/clap-config
Basically, it would be pretty cool to generate a simple config file parser from a clap argument parser. There's a longer description of some use cases in the readme, but it would be great for projects which are configurable with both the command line and a config file, and for helping projects add config file configuration in addition to command line configuration.
The text was updated successfully, but these errors were encountered: