Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support root level nested value lookups #16

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lukasbindreiter
Copy link

What type of PR is this?

  • bug

What this PR does / why we need it:

It fixes a bug in NestedVal where lookup doesn't work for keys directly in the first level (root) of the map.

Which issue(s) this PR fixes:

Fixes #14

Special notes for your reviewer:

The issue is related to this one I raised in cli: urfave/cli#2037
I created a related PR there as well: urfave/cli#2047

The source code of NestedVal, and even the tests seem to be a copy paste from urfave/cli in this case.
I kept it now like this as well - though there would be a possibility to remove that duplication by moving the NestedVal function to urfave/cli directly, make it public and use it there inside MapSource.Lookup and then also in cli-altsrc.

If you prefer that approach I could update my PRs accordingly - though this would require the urfave/cli one to be merged and released before this PR can be updated to rely on it.

Testing

I added a test case for this exact issue as well. It will fail without changes from the PR.
I also added another test case for the map[string]any special case that was in place in the code, to make sure that works to.

Copy link
Contributor

@dearchap dearchap left a comment

Choose a reason for hiding this comment

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

@lukasbindreiter Amazing work. Thanks for the PR. !!!

@lukasbindreiter
Copy link
Author

@dearchap CI was failing due to some unrelated test case - connecting to machines on a local network seems to be disabled on Github CI - I attempted to fix it by changing the expected error to a substring that covers both cases.

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.

Lookup of values from JSON/TOML/YAML doesn't work for root properties - only at least one level nested one
2 participants