fsprojects / Argu

A declarative CLI argument parser for F#
https://fsprojects.github.io/Argu
MIT License
453 stars 75 forks source link

appsetting.json and NET5 #171

Open jkone27 opened 2 years ago

jkone27 commented 2 years ago

aspnetcore and dotnet core app config has been moved to appsettings.json for a while, does argu support binding on that too?

Junocaen commented 2 years ago

You can give this repo a try: https://github.com/Tarmil/Argu.MicrosoftExtensions.

bartelink commented 9 months ago

Would either of you (or @toburger) consider doing a PR that means the average user will be aware there is a valid approach? Perhaps a PR that:

(My concern here is this issue still sitting here in 2030 without anythign having moved forward - unpacking the above comments is definitely someone new to F# and/or .NET would not find trivial!)

Numpsy commented 8 months ago
  • and ideally we'd even lose the System.ConfigurationManager ref, not be adding more of course

A quick play with the code suggests that moving AppSettingsConfigurationReader and such out or the main assembly should be doable, except for the default IConfigurationReader fall back at

https://github.com/fsprojects/Argu/blob/feca25aad5bb2cabb2f19a7b4d106f36f41ea9a2/src/Argu/ArgumentParser.fs#L171

that wouldn't work any more in that case.

(Note: I haven't used Argu yet, I was just looking at alternatives to (FSharp.)SystemCommandLine and thinking that a command line parser depending on the System.Configuration.ConfigurationManager stack isn't ideal)

Numpsy commented 8 months ago
  • maybe does something that might be a reasonable OOTB addition here?

What sort of thing are you thinking of there? It looks like the referenced Argu.MicrosoftExtensions is binding to Microsoft.Extensions.Configuration.IConfiguration rather than using Appsettings.json directly, but maybe that is a generic / round about way of getting both Json and TOML support? (i.e maybe you could have a lib that binds IConfiguration to Argu directly, but doesn't touch the other Microsoft.Extensions.DependencyInjection stuff)

bartelink commented 8 months ago

What sort of thing are you thinking of there?

I personally have no interest in reading config files, much less having hardwired support for only one built in. My generic statement is intended to be open-ended, but I guess I was alluding to trade-offs like:

I resolve those forces via https://github.com/jet/dotnet-templates/blob/master/feed-consumer/Program.fs#L94 but that's hard-won, and there's definitely scope to put something in the repo, with docs that's sufficiently extensible.

Given the build tooling is pretty decent, it would not be a problem to push out the config dependencies to an external Argu.AppSettings package

The problem though, is to work out an abstraction that covers the above, i.e.:

Solving all of that is probably out of scope for this issue (some of these desires are covered in other issues). There's definitely room to so something "good enough for now" that moves things forward.

If you and/or others have appetite, we could potentially do a -beta release cycle that trials something in a useful direction. A small breaking change such as e.g. moving out app settings attributes into another assembly might be viable in such a context.

I'm happy to muck in a bit and will definitely review things, but I'm reasonably well loaded atm with things that Need To Be Done in other projects so am not in a position to drive it. I'd definitely be able to validate a wide variety of consumption scenarios.

In other words, if you have some capacity, there's a lot of potential - go for it!

Numpsy commented 8 months ago

Hmm, I hadn't thought about anything to that detail (I was just looking at the command line parsing functionality, without any other sort of configuration), so I'd have to see if I have any time in the new year for anything further.

As far as the ConfigurationManager dependency goes, I was just playing around with something like https://github.com/Numpsy/Argu/commit/5b94ad7cff1bb4dd699fb45445673fd64a178958, which seemed like it worked with the same public api, except the point about not working as a 'default' functionality any more

bartelink commented 6 months ago

Sorry, only coming back to this now - your spike seems good.

I suspect the massive majority Argu consumption scenario is massively weighted toward only using explicit parsing of the command line. It would obv not be surprising if making the file parsing opt-in is a perf win too.

The main concern is that the behavior change would be breaking - all this really means is that it would need to wait for a V7. So making a formal PR out of it might be good? Ideally there'd be some way to signal the change with a warning or obsoletion rather than relying on doc stuff that lots of people will miss and will then get bitten by. I personally won't have the time or appetite to go full in on doing a full job of letting people arbitrarily compose the read (and help generation) wiring (and ordering) as I alluded to earlier, but at least making the config parsing opt-in might be a good first step.

I normally have a helper in my Arguments type like this

    static member Parse argv =
        let parseResults = ArgumentParser.Create<Parameters>().ParseCommandLine argv
        Arguments(Args.Configuration(EnvVar.tryGet, EnvVar.getOr parseResults.Raise), parseResults)        

or a

module Arguments =
    let parse argv = ArgumentParser.Create<Arguments>().ParseCommandLine(argv)

If that became a one-liner such as Argu.ArgumentParser.Create().ParseCommandLineOnly<Parameters>(argv), then the secondary package could augment with Argu.ArgumentParser.Create().ParseAppSettingsFallback<Parameters>(argv), and we could obsolete the .Create with a mention of needing to ref the other package if you want the appsettings too. The Create can then remain (but Obsolete) with the old program name derivation logic until V8.

Other light breaking changes like https://github.com/fsprojects/Argu/issues/211 might be worth considering alongside for V7.

Then perhaps a V8 can do deeper stuff like making it pluggable, but as each supported has its own factory method, those can forward to a more generic pipeline factory thing.

It might also be nice to do an equivalent of https://github.com/fsprojects/FSharp.AWS.DynamoDB/pull/73 in the V7 timeframe, as the work to make the pipeline pluggable might involve some diffs that could be painful to review if we also have formatting inconsistencies in the mix)

Any thoughts @nojaf ?

bartelink commented 6 months ago

Also noticed that the argv is optional. For V7, there should be one with a clear name and a mandatory argv, and another named to convey the behavior if you omit. Also I am considering to flip the default true of the raiseOnUsage to false and renaming it to supressRaiseOnUsageRequest.

image
nojaf commented 6 months ago

Any thoughts @nojaf ?

Not many to be honest. It is probably worth exploring in v7 when breaking changes are on the table.