Skip to content

Conversation

@dearchap
Copy link
Contributor

What type of PR is this?

(REQUIRED)

  • documentation
  • feature

What this PR does / why we need it:

(REQUIRED)

In order to make the cli-altsrc more modular in terms on its provider it seems to be better if map value source can be incorporated directly into cli.

Which issue(s) this PR fixes:

(REQUIRED)

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

go test -run=TestMapValueSource

Release Notes

(REQUIRED)

New value source for maps has been included. This allows plugin providers for other formats(json, yaml, etc) to utilize base functionality instead of each source do custom nested lookups

@abitrolly
Copy link
Contributor

abitrolly commented Nov 21, 2024

Is it the "map value" or the map of value sources?

For debugging and bug reports it might be useful to have a structure and output that shows where option is sets, and which value is selected finally. Like:

$ CLI_PATH=/eee cli --path=/ccc --debug
urfave/cli: --path (default: none, env: /eee, cmdline: /ccc, final: /ccc)

@dearchap
Copy link
Contributor Author

Is it the "map value" or the map of value sources?

For debugging and bug reports it might be useful to have a structure and output that shows where option is sets, and which value is selected finally. Like:

$ CLI_PATH=/eee cli --path=/ccc --debug
urfave/cli: --path (default: none, env: /eee, cmdline: /ccc, final: /ccc)

It is a map-value source i.e a source which can lookup values based on key.

Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @abitrolly that mapValueSource is a bit confusing, but I follow your reasoning 😅👍🏻

@dearchap dearchap merged commit 0a1c772 into urfave:main Nov 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants