Refactor flag handling, add tests - #9
Open
elenangreen wants to merge 13 commits into
Open
Conversation
Remove the cli-altsrc dependency. TOML and environment variable resolution now uses the built-in MapSource and ValueSourceChain. The cli-altsrc library only supported a single TOML source. Restructure args into a separate package (matching Airlock): - CLIArgs is the superset struct composing the arguments for all subcommands - GlobalFlags/OpenFlags return reusable flag sets keyed by subcommand - TOML defaults flow through MapSource in the Sources chain after environment variables `config` and `nodefaults` handled through a new two-step parsing process to break a dependency loop (need to resolve both flags to determine which TOML files to add to the ValueSourceChain). Implemented as a minimal `cli.Command` in main.go that only parses these two arguments. All command handlers use a passed *args.CLIArgs, eliminating the previous need for package-level globals.
new internal/cmd/bootstrap package that only handles these flags.
polish: add and update comments
Add tests for the new flag source system to verify the resolution order and wiring. - args_test.go: flag definitions, destinations, validators, and hidden flag properties for GlobalFlags and OpenFlags - sources_test.go: flagSources() chain structure and env/TOML resolution at the unit level - integration_test.go: end-to-end value resolution through cli.Command.Run() covering CLI > env > TOML precedence - bootstrap_test.go: bootstrap.Run() file loading, --config handling, and system config path - wh_test.go: wh.Tasks() command structure and globalWrap behavior - helpers.go: shared TOMLEntry/TOMLEntries test helpers
Addresses feedback from LLM. - TestOpenFlags: validate Sources/Category/TrimSpace on all flags instead of hardcoding names, counts, and properties. - TestGlobalWrap_BasicBehavior: direct assertions instead of table-driven any-capture pattern. - Add TestIntegration_RequiredFlagMissing_WithSources. - Rename bootstrap Configs/NoDefaults to lowercase. - Remove redundant "count" subtest from TestRun_MultipleUserConfigs. - Polish convertTopLevel comment.
Addresses feedback from LLM.
Update documentation to use hyphens for config parameters
Addresses LLM feedback. - Assert bootstrap precedence via resolved values instead of slice ordering - Remove urfave/cli chain-shape assertions from flag tests - Add bootstrap test for WORMHOLE_NODEFAULTS with explicit user config - Restore command wiring tests for open flags
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactored and reorganized flag handling code to improve testability and consistency. Added unit tests and basic integration tests for the new flag handling code.
Breaking change: TOML parameters now follow CLI flags (hyphens rather than underscores in parameter names)