feat(Async): Add IAsyncConfigurationReader + ParseAsync overload#317
feat(Async): Add IAsyncConfigurationReader + ParseAsync overload#317dimension-zero wants to merge 26 commits into
Conversation
* ConfigReaders.fs:
- New IAsyncConfigurationReader interface mirroring
IConfigurationReader with GetValueAsync : string -> Task<string>.
- ConfigurationReader.AsAsync(reader) wraps any sync reader as async
via Task.FromResult — useful when callers want to pass existing
readers through the async parse path.
- ConfigurationReader.FromAsyncFunction(asyncFn, ?name) builds a
reader from an F# Async<string option>.
* ArgumentParser.fs new ParseAsync overload: fetches every top-level
AppSettings key the schema references via the async reader, awaits
each, then runs the regular sync Parse against a Dictionary-backed
snapshot. One round-trip per declared key; below ParseAsync, the
parser stays purely synchronous so existing tests and behaviour are
unaffected.
Hosts that previously had to block on an async config source can now
use ParseAsync directly:
do! parser.ParseAsync(configurationReader = remoteAsync) |> Async.AwaitTask
5 new tests: * ConfigurationReader.AsAsync wraps a sync reader; the wrapped Task resolves to the same value and preserves Name. * ParseAsync via AsAsync(sync reader) yields the same parse results as the original sync Parse against that reader. * ParseAsync pre-fetches each schema-declared AppSettings key exactly once (Interlocked-counted via a custom IAsyncConfigurationReader), guarding the implementation's contract. * ConfigurationReader.FromAsyncFunction adapts an F# Async<string option> to IAsyncConfigurationReader and parses round-trip. * ParseAsync forwards ignoreUnrecognized to the underlying sync Parse (CLI tokens not in the schema land in UnrecognizedCliParams). Net suite size on this branch: 112 -> 117.
risk: LOW (score: 0.0, no analysable symbols)
|
@bartelink do you have some bandwidth to review these PRs? |
|
lol, did T-Gro mention I'm atting him too much and need to take a break :D The PRs thankfully look pretty reviewable but ngl I was hoping they'd magically get merged by 'someone'! But yes, I do have space atm so I'll chip away at them |
There was a problem hiding this comment.
Pull request overview
This PR adds async configuration reader support to Argu so callers can parse command-line arguments with configuration values sourced asynchronously.
Changes:
- Adds
IAsyncConfigurationReader, async adapter helpers, and aParseAsyncoverload. - Adds tests covering async reader adapters and ParseAsync behavior.
- Adds unrelated
.gitignoreand.recodegenerated-tooling changes.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/Argu/ConfigReaders.fs |
Adds async configuration reader API and factory/adapters. |
src/Argu/ArgumentParser.fs |
Adds ParseAsync that prefetches async config values then delegates to sync parsing. |
tests/Argu.Tests/ParseAsyncTests.fs |
Adds tests for async reader and parser behavior. |
tests/Argu.Tests/Argu.Tests.fsproj |
Includes the new async parse test file. |
.gitignore |
Adds additional ignore rules, including duplicated C#/F# blocks. |
.recode/get-recap/recap.md |
Adds generated local recap artifact. |
.recode/get-recap/sessions/*.json |
Adds generated local session artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* ConfigReaders.fs:
- New IAsyncConfigurationReader interface mirroring
IConfigurationReader with GetValueAsync : string -> Task<string>.
- ConfigurationReader.AsAsync(reader) wraps any sync reader as async
via Task.FromResult — useful when callers want to pass existing
readers through the async parse path.
- ConfigurationReader.FromAsyncFunction(asyncFn, ?name) builds a
reader from an F# Async<string option>.
* ArgumentParser.fs new ParseAsync overload: fetches every top-level
AppSettings key the schema references via the async reader, awaits
each, then runs the regular sync Parse against a Dictionary-backed
snapshot. One round-trip per declared key; below ParseAsync, the
parser stays purely synchronous so existing tests and behaviour are
unaffected.
Hosts that previously had to block on an async config source can now
use ParseAsync directly:
do! parser.ParseAsync(configurationReader = remoteAsync) |> Async.AwaitTask
# Conflicts:
# src/Argu/ConfigReaders.fs
5 new tests: * ConfigurationReader.AsAsync wraps a sync reader; the wrapped Task resolves to the same value and preserves Name. * ParseAsync via AsAsync(sync reader) yields the same parse results as the original sync Parse against that reader. * ParseAsync pre-fetches each schema-declared AppSettings key exactly once (Interlocked-counted via a custom IAsyncConfigurationReader), guarding the implementation's contract. * ConfigurationReader.FromAsyncFunction adapts an F# Async<string option> to IAsyncConfigurationReader and parses round-trip. * ParseAsync forwards ignoreUnrecognized to the underlying sync Parse (CLI tokens not in the schema land in UnrecognizedCliParams). Net suite size on this branch: 112 -> 117.
risk: LOW (score: 0.0, no analysable symbols)
Generated session files containing local machine/account metadata are not project source. Add .recode/ to .gitignore so they are never accidentally committed again. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…c behaviour parseKeyValuePartial guards GetValue with `try ... with _ -> null`. ParseAsync had no equivalent guard so a throwing async reader would fail the entire call. Wrap GetValueAsync in the same pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This reverts commit 3acc425.
128f27c to
4027d0f
Compare
3e496b4 to
35786cd
Compare
bartelink
left a comment
There was a problem hiding this comment.
Do you have a concrete use case for this or is this more a conceptual thing? Unless there is a direct usage that you'll apply it to in the short term, my preference would be to let this sit and sort out integration with other sources e.g. M.E.C [and/or deprecation/externalization of sources that have fallen out of usage] first
Assuming you do have a concrete use case, IMO such a powerful feature (that brings the aspects of a distributed system into play) warrants a sample, which would also address copilot's review comment about exception handling, and also covering whether an ArgumentParser.TryCreate fits in, i.e. covering:
- startup, how do we handle ArguException for mangled/unsupported attribute coimbinations
- parsing, ArguParseException for actual bad values from user or config store input
- handling/ reporting failure to contact config store
- potentially provide a way for the remote store to have a default strategy in the case where the server is uncontactable - i.e. could/would you let the remote store overrides all return
nullif the store was unavailable, but write tostderr?
A readme should also flag alternate ways of accomplishing the same thing (i.e. various config aggregation systems at .NET and/or broader application hosting system level)
As it stands, I get the general concept, but the above raises too many questions, and this feature starts with -100 points as it increases the API surface and concept count significantly
If there's a decent sample and a readme that covers the above concerns, my perspective may shift
risk: LOW (score: 0.0, no analysable symbols)
Per Copilot's review on fsprojects#317, the .recode/get-recap/*.json session files contain local machine and account identifiers from per-developer tooling and don't belong in source control. Removed the seven existing artifacts and added .recode/ to .gitignore so future captures stop leaking into the repo.
Per @bartelink's review on fsprojects#317: the per-key GetValueAsync interface forces N round-trips for an N-key schema even when the underlying source (key/value store, secrets vault, remote config server) supports batch retrieval natively. Collapsed the contract to a single batched call. * IAsyncConfigurationReader.GetValueAsync -> GetValuesAsync - Takes IReadOnlyCollection<string> - Returns Task<IReadOnlyDictionary<string, string>> - Missing keys are absent from the dictionary (matches the per-key null contract of IConfigurationReader.GetValue) * ConfigurationReader.AsAsync now satisfies the batched contract by looping per key against the wrapped sync reader. * ConfigurationReader.FromAsyncFunction bridges from per-key async sources (HTTP GET ?key=); doc string flags that batching natively collapses the N round-trips. * New ConfigurationReader.WithFallbackToNull: wraps any async reader so transport faults degrade to 'all keys missing' rather than failing the parse, with a configurable onFault hook (defaults to a single stderr line). Addresses @bartelink's 'best-effort when the store is unavailable' concern explicitly. * ArgumentParser.ParseAsync issues a single batched GetValuesAsync for every AppSettings-mapped top-level case in the schema; ParseAsync XML doc documents the failure modes (faulted Task = fatal startup error unless wrapped in WithFallbackToNull). Tests updated to the batched contract: - AsAsync wraps + missing-key behaviour - ParseAsync matches sync Parse against the same dictionary - ParseAsync issues exactly one batched call observing all schema keys - FromAsyncFunction adapts an F# per-key async function - Bare faulted reader propagates the exception out of ParseAsync - WithFallbackToNull downgrades the fault and CLI args still satisfy - ignoreUnrecognized passes through 150 tests pass.
…de docs Addresses @bartelink's three remaining concerns on fsprojects#317: * samples/Argu.Samples.AsyncConfig/ - end-to-end CLI demonstrating ParseAsync against a real source (Azure Key Vault via the Azure.Security.KeyVault.Secrets SDK). - Real reader uses DefaultAzureCredential and Task.WhenAll over the requested keys to collapse N per-secret calls into one logical round-trip; 404s on individual secrets are treated as missing. - --simulate true (default) runs against an in-process fake reader so the sample is runnable without Azure credentials; --simulate false hits the real vault using the --vault-url CLI arg. - Entry point explicitly demonstrates the three failure modes: ArguException at Create (programmer error), faulted Task from GetValuesAsync (deploy error - wrapped via WithFallbackToNull), and ArguParseException from the sync parse phase (user error). * docs/tutorial.fsx - new 'Async Configuration Sources' section covering: - When to reach for ParseAsync vs. M.E.C. / Argu.Extensions.Configuration vs. host-level resolution. - The batched contract (one round-trip when the source supports it). - The three-flavour failure model and WithFallbackToNull policy. * Argu.slnx - registers the new sample under the existing samples folder. * Directory.Packages.props - centrally pins Azure.Security.KeyVault.Secrets 4.7.0 and Azure.Identity 1.13.1 (sample-only; not referenced from the core package).
|
@bartelink — full lap through your review. Pushed three commits:
The "best-effort if the store is unavailable" ask is now On Full suite (150 tests) green. Ready for re-review. |
|
@bartelink — bumping for a re-review now that the redesign is in. You asked for a concrete use case before merging an async config surface. The new Since your review:
CI green across all four jobs; all 150 tests pass. |
|
From the overview comments, all sounds good. Key outstanding concern is how we marry Async with N layers of fallback as I muse about in #325 (comment) If this PR can land basic samples and/or tutorial sections that convey:
As things get more advanced, I think there's less need for them to be in the tutorial or docs, as long as we have samples and the samples are compiled as part of the build as a cross check. Will review this in full and/or merge pretty soon unless others have concerns edit: quick scroll through suggests you're well on top of the above and have anticipated most of those aspects. (It also sees a lot of flotsam in the .gitignore - can you keep it just to the recode and/or stuff you use or we'll join every other repo on the planet in having a history of all the tools that have existed during our lifetimes! (*.sln.docstates jumps out as something that's not needed as we're slnx now) |
|
Sure, |
Addresses review feedback that the .gitignore had accumulated flotsam. The branch had appended ~340 lines: a generic VS/dotnet template duplicated verbatim under both "# csharp" and "# fsharp" headers, plus bash/OS-IDE blocks and a stray re-add of *.sln.docstates that contradicted the *.slnx.docstates entry already present (the repo is slnx-only now). Master's existing rules already cover bin/, obj/, *.nupkg, .vs/, /.idea, packages/ and the rest, so the dump was pure redundancy. Removed it, keeping only the two genuinely repo-specific changes: - *.sln.docstates -> *.slnx.docstates (slnx modernisation) - .recode/ (local Recode tooling artifacts)
|
@bartelink — two things addressed.
Sample/tutorial tiers. Mapping your four cases onto what's here:
All three sample projects are registered in On the bigger picture from #325 (collapsing the overlapping |
Resolves the Directory.Packages.props conflict additively: keeps both the upstream BenchmarkDotNet entry (merged via fsprojects#313) and this branch's Azure Key Vault sample dependencies. Full solution builds clean; 150 tests pass.
| let reader = | ||
| let handle (ex: exn) = | ||
| log.Warn("vault unavailable: {0}", ex) | ||
| readOnlyDict Seq.null |
There was a problem hiding this comment.
could do with adding some values here
| | [<CustomAppSettings("db-host")>] Db_Host of host:string | ||
| | [<CustomAppSettings("port")>] Port of port:int | ||
| | [<CustomAppSettings("feature-flag")>] Feature_Flag of name:string | ||
| | [<Unique>] Simulate of value:bool |
There was a problem hiding this comment.
dont use bool, just check for the flag with .Contains
| return Some (k, resp.Value.Value) | ||
| // Simulate handling of individual read failures - note we are not trapping generic connectivity errors | ||
| // For such cases, ConfigurationReader.WithFallback | ||
| with :? RequestFailedException as ex when ex.Status = 404 -> |
There was a problem hiding this comment.
I had to read other stuff to make sense of this - obviously retrieving all secrets is bad news but need to convey that you don't want to be doing individual roundtrips
| with :? RequestFailedException as ex when ex.Status = 404 -> | ||
| return None | ||
| } } | ||
| let! all = Task.WhenAll tasks |
There was a problem hiding this comment.
F# 11 will hopefully gain a Task.parallelLimit to manage this...
| let reader = | ||
| let handle (ex: exn) = | ||
| eprintfn $"WARNING: %s{asyncReader.Name} unavailable; CLI defaults only (%s{ex.Message})" | ||
| readOnlyDict Seq.empty // TOCONSIDER could provide a set of fallback values |
| eprintfn $"%s{ex.Message}" | ||
| exit 1 | ||
|
|
||
| printfn $"""Resolved configuration: |
There was a problem hiding this comment.
note sure it's for the better with this but anyway...
| printfn $"""Resolved configuration: | ||
| source = %s{reader.Name} | ||
| db-host = %s{results.GetResult(Db_Host, defaultValue = "<missing>")} | ||
| port = %s{results.TryGetResult(Port, string) |> Option.defaultValue "<missing>")} |
There was a problem hiding this comment.
consider %s(GetResult(Port, 80) or similar?
| /// <param name="raiseOnUsage">Treat '--help' parameters as parse errors. Defaults to <c>true</c>.</param> | ||
| member x.ParseAsync (inputs : string[], configurationReader : IAsyncConfigurationReader, ?ignoreMissing, ?ignoreUnrecognized, ?raiseOnUsage) : Task<ParseResults<'Template>> = task { | ||
| // Collect every AppSettings key the top-level schema references. | ||
| // The sync parser walks subcommand-local schemas separately, so only top-level keys go in this batch. |
There was a problem hiding this comment.
Hm but there's no wiring for another roundtrip to go async to the source (how does that work/how do you propose to resolve it if there are potentially multiple?)
|
|
||
| type Args = | ||
| | TagKey of string | ||
| | [<CustomAppSettings("port-key")>] PortKey of int |
There was a problem hiding this comment.
this Attribute name was surprising to me
but it explains a lot
You'll note I removed the local app settings fallback from the new API
I think the best thing is for us to (similar to how we arrived at a neutral Separator attribute) to end up with:
CustomAppSettings->Setting,ConfigKey,SettingsKeyorConfigNamefor tagging env vars, AppSettings or anything else- ArgumentParser.ParseAsync as here has mandatory argv and mandatory async config source (which people can wire a local config into the tree if they want - as mentioned I bet few will do that)
- ArgumentParser.ParseCommand is argv only with no config
- ArgumentParser.Parse has mandatory argv and mandatory config reader
- arguably the ParseCommand overloads should reject a DU if they have
Settingannotations
Then the missing pieces are:
- for the usage strings and/or failure messages to convey for a value what the settings key and the argument name(s) are for any given parameter
- remove any implicit lookups or defaulting from env vars, settings or anywhere else
- maybe reinstate ParseConfig if it still makes sense in the end (I have my doubts though)
…osoftConfiguration (fsprojects#308) * feat(Config): Add env-var prefix factory + Argu.MSConfig companion * ConfigReaders.fs: New static member ConfigurationReader.FromEnvironmentVariables(prefix : string) that reads env vars with a fixed prefix prepended to the key. Composed from the existing EnvironmentVariableConfigurationReader via the existing FunctionConfigurationReader, so no new types in core. Existing FromEnvironmentVariables() is untouched. * New project src/Argu.MSConfig: thin adapter exposing any Microsoft.Extensions.Configuration.IConfiguration as an Argu IConfigurationReader. Lives in its own NuGet package so the core stays zero-dep on Microsoft.Extensions.*. * Directory.Packages.props: centrally-managed pin for Microsoft.Extensions.Configuration.Abstractions 8.0.0. * Argu.sln: register the new project under the existing F# tooling configuration. * test(EnvVarPrefix): Add coverage for FromEnvironmentVariables(prefix) 4 new tests: * FromEnvironmentVariables(prefix) prepends the prefix to the requested key, so reader.GetValue('HOST') reads env var MYAPP_HOST. * Missing keys come back as null (Argu's standard contract). * Round-trip through ArgumentParser.ParseConfiguration: a schema using CustomAppSettings keys is populated correctly when only the prefixed env vars are set. * No-arg FromEnvironmentVariables() is still functional (the new overload doesn't shadow the legacy one). Net suite size on this branch: 112 -> 116. * refactor: Rename Argu.MSConfig -> Argu.Extensions.Configuration Per review on fsprojects#308, the M.E.C-style name matches the rest of the Microsoft.Extensions.* ecosystem and avoids the two-letter capitalised 'MSConfig' segment. * src/Argu.MSConfig/ -> src/Argu.Extensions.Configuration/ * Argu.MSConfig.fsproj -> Argu.Extensions.Configuration.fsproj (AssemblyName / RootNamespace / PackageId all default to the project file name) * Namespace Argu.MSConfig -> Argu.Extensions.Configuration in ConfigurationReader.fs * Solution registration updated in Argu.slnx * test(EnvVarPrefix): Scope env overrides via use _ = envOverride (IDisposable) Per review feedback: replace the withEnv wrapper-function helper with an IDisposable-returning envOverride, so each test reads `use _ = envOverride key value` and the prior value is restored in Dispose(). Removes the nested-lambda indentation in the multi-var round-trip test. --------- Co-authored-by: Ruben Bartelink <ruben@bartelink.com> # Conflicts: # src/Argu/ConfigReaders.fs
f81623b to
93fd6a9
Compare
# This is the 1st commit message: Review # The commit message #2 will be skipped: # f
…osoftConfiguration (fsprojects#308) * feat(Config): Add env-var prefix factory + Argu.MSConfig companion * ConfigReaders.fs: New static member ConfigurationReader.FromEnvironmentVariables(prefix : string) that reads env vars with a fixed prefix prepended to the key. Composed from the existing EnvironmentVariableConfigurationReader via the existing FunctionConfigurationReader, so no new types in core. Existing FromEnvironmentVariables() is untouched. * New project src/Argu.MSConfig: thin adapter exposing any Microsoft.Extensions.Configuration.IConfiguration as an Argu IConfigurationReader. Lives in its own NuGet package so the core stays zero-dep on Microsoft.Extensions.*. * Directory.Packages.props: centrally-managed pin for Microsoft.Extensions.Configuration.Abstractions 8.0.0. * Argu.sln: register the new project under the existing F# tooling configuration. * test(EnvVarPrefix): Add coverage for FromEnvironmentVariables(prefix) 4 new tests: * FromEnvironmentVariables(prefix) prepends the prefix to the requested key, so reader.GetValue('HOST') reads env var MYAPP_HOST. * Missing keys come back as null (Argu's standard contract). * Round-trip through ArgumentParser.ParseConfiguration: a schema using CustomAppSettings keys is populated correctly when only the prefixed env vars are set. * No-arg FromEnvironmentVariables() is still functional (the new overload doesn't shadow the legacy one). Net suite size on this branch: 112 -> 116. * refactor: Rename Argu.MSConfig -> Argu.Extensions.Configuration Per review on fsprojects#308, the M.E.C-style name matches the rest of the Microsoft.Extensions.* ecosystem and avoids the two-letter capitalised 'MSConfig' segment. * src/Argu.MSConfig/ -> src/Argu.Extensions.Configuration/ * Argu.MSConfig.fsproj -> Argu.Extensions.Configuration.fsproj (AssemblyName / RootNamespace / PackageId all default to the project file name) * Namespace Argu.MSConfig -> Argu.Extensions.Configuration in ConfigurationReader.fs * Solution registration updated in Argu.slnx * test(EnvVarPrefix): Scope env overrides via use _ = envOverride (IDisposable) Per review feedback: replace the withEnv wrapper-function helper with an IDisposable-returning envOverride, so each test reads `use _ = envOverride key value` and the prior value is restored in Dispose(). Removes the nested-lambda indentation in the multi-var round-trip test. --------- Co-authored-by: Ruben Bartelink <ruben@bartelink.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
93fd6a9 to
f0afaf3
Compare
risk: LOW (score: 0.0, no analysable symbols)
feat(Async): Add IAsyncConfigurationReader + ParseAsync overload
IConfigurationReader with GetValueAsync : string -> Task.
via Task.FromResult — useful when callers want to pass existing
readers through the async parse path.
reader from an F# Async.
AppSettings key the schema references via the async reader, awaits
each, then runs the regular sync Parse against a Dictionary-backed
snapshot. One round-trip per declared key; below ParseAsync, the
parser stays purely synchronous so existing tests and behaviour are
unaffected.
Hosts that previously had to block on an async config source can now
use ParseAsync directly:
do! parser.ParseAsync(configurationReader = remoteAsync) |> Async.AwaitTask
test(ParseAsync): Coverage for IAsyncConfigurationReader + ParseAsync
5 new tests:
resolves to the same value and preserves Name.
the original sync Parse against that reader.
once (Interlocked-counted via a custom IAsyncConfigurationReader),
guarding the implementation's contract.
to IAsyncConfigurationReader and parses round-trip.
(CLI tokens not in the schema land in UnrecognizedCliParams).
Net suite size on this branch: 112 -> 117.