About the clap defaults, I think it might be possible to keep using them if we use value_source to detect if it was a default or CLI-provided argument. On the TOML side, you'd have to something like parse into a toml::Table, record the keys, then parse the table into a lychee Config.

But if all this works (and I don't know if it will, it's all just a theory from reading docs), then the merge function would take two configs and lists of the user-defined keys for each, then it merge and prioritise using that information.

Idk how much work this would be, but it would be really nice to keep using the clap default system.

@katrinafyi Oh wow, thank you so much for the suggestion. I really missed that, or rather never consulted the docs properly enough 😅 I will definitely explore this path, thank you for pointing it out.

So I've updated this PR with 4d4fc89. We can now specify --config multiple times, which is much cleaner than the confusing and overly-specific --secrets option.

Clap defaults

Clap defaults are removed in the current state of the PR. This is because leaving them leads to incorrect behaviour. We want clap arguments to take precedence over config values, otherwise UX is quite confusing IMO. It would be much easier if this wasn't the case, but I think it really is unexpected to have CLI flags which are overwritten by values in config files.

graph LR;
    Config-A-->Config-B;
    Config-B-->Clap;

There can be any amount of configs, this is just an example with two configs. Config-A might be the main config, tracked with git. Config-B might contain only a few sensitive values (e.g. header values), being specifically excluded from git tracking.

If we decided to leave the clap default values, then if Config-A or Config-B specifies e.g. format: Some(Json) then Clap would overwrite it with the default value of Some(Compact). Previously we just ignored any values coming from Clap if they were equal to the default value, meaning if a user uses --format compact it was ignored. This is #1959.

I've experimented with macros in thomas-zahner@605fe31 where I try to auto generate the "getter" methods and assert that the right default value is mentioned in the comment. But unfortunately it's not possible to evaluate const expressions in macros so that it's impossible to assert the default value is contained in the comment when it is a const literal. Also this seems to be quite overkill.

Then I've tried your approach @katrinafyi in thomas-zahner@dbd36ce#diff-6168b1b6485695baf19bdea9ca3993b2c1f705821ab33f53775ad279ce2f6badR146. But this does not seem to work well. I see None, even though I specified the argument via CLI flag. Also this approach seems to be very error prone and tedious, as the arguments are referred to by string and we get runtime panics if we try to obtain infos for IDs which don't correspond to any argument.

I'm open for ideas. Currently, I feel like the state of this PR is pretty much the most realistic version. Ideally I will write some additional tests, which might be able to assert that manually typed out default values (e.g. /// [default: compact]) are actually accurate. Not sure how easy this is.

@thomas-zahner with value source, was it particular flags which caused that problem? I was just testing it and it seems to work. I don't know what causes None vs Some(DefaultValue), but Some(CommandLine) does seem to be returned correctly as far as I can tell.

But maybe I'm just rewalking the path you already tried. The API could definitely be safer and less stringy..

@katrinafyi Hmm, it basically worked for not a single argument in my test. See https://github.com/thomas-zahner/lychee/tree/clap-value-source-v2

Could you share your code if you got it working?

Coincidentally, I just discovered the same issue 😆 It's really confusing. value_source stops working (and always returns None) after from_arg_matches_mut is used. I figure it mutates the arg matches to lose the value source information?? It works if you call value_source before parsing the ArgMatches into LycheeOptions.

My code is at https://github.com/lycheeverse/lychee/compare/master..rina-forks:lychee:value-source. It does the merging by lowering to TOML and then it can use the TOML map to represent presence/absence. But it's a real headache with all the string keys and untyped toml AST.

I think your approach is probably the least bad. Even if value_source was working, the string keys are a real bother. I also tried writing macros to make it more type-safe, but you inevitably have to deal with strings at the boundaries. ce60036

edit: or maybe the macro is ok. the hard part is just defining is_defined predicates for the CLI and TOML. this could be done with helper function that maps field enum to string name. we can check that is safe by taking the image and checking subset of serde names or clap names. then we can keep merging at the type-safe level too........

My code is at https://github.com/lycheeverse/lychee/compare/master..rina-forks:lychee:value-source. It does the merging by lowering to TOML and then it can use the TOML map to represent presence/absence. But it's a real headache with all the string keys and untyped toml AST.

Oh wow, that looks like you put quite some work into it. But it also seems really complicated 😅

I've just come up with three tests in 3303cc0. What do you think? It seems much simpler and pragmatic to go with such conventions. And now as we test them it shouldn't be a problem. It also has the benefit that we show even more default values to the user. (e.g. as with archive)

But it also seems really complicated 😅

Yeah... I was following different threads at the same time so could be a little bit simpler, but it's definitely more complicated than I wanted or expected.

It's just... I really don't like all the getters to apply the defaults - it's just so Java 😭 But it's something we'll have to deal with.

Anyway, the tests you wrote look good and make sense. Let me know when it's ready and you want me to look through, I had some small comments queued.

I might or might not rebase my changes after your PR is merged to see what it's like. Or maybe not if it's too much work, we'll see..

It's just... I really don't like all the getters to apply the defaults - it's just so Java 😭 But it's something we'll have to deal with.

Haha yeah, I see your point. But at the same time I think it's an overall improvement, as we document more default values (accept & archive) than before and any mistakes should be prevented through the new tests. Also it's really low complexity, no macro magic.

Overall the approach is pretty good now in my opinion. If you look at other bigger CLI applications you will see funny things, how they implement CLI parsing fully on their own without any library (ripgrep which switched from clap to their own implementation) or how they have duplicated structs one for CLI args and one for the config file. (fd)

The PR is now ready for review.

@katrinafyi Thank you for the thorough review!

A really small thing but were these two comments meant to make it in #2007 (comment) #2007 (comment)? Or did you choose to leave them out?

@katrinafyi Oh, that's what happens when you apply all review suggestions and then address the test failures by copying the diff blindly from the test output 😄 (I assumed there were no suggestions in the README)

See 5500879

Aha I see, that's so fair. I should put the suggestions in the rust side next time :))