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.

Sign up for GitHub

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

Jump to bottom

IsSet() returns false when flags are not set #331

Merged
merged 2 commits into from
Nov 29, 2019
Merged

IsSet() returns false when flags are not set #331

merged 2 commits into from
Nov 29, 2019

Conversation

benoitmasson
Copy link
Contributor

@benoitmasson benoitmasson commented Apr 11, 2017 •
edited by sagikazarmark
Loading

Added flexibility to find() function with an extra boolean argument, so that it can be used both by Get() (to retrieve a value, including flag's default) and IsSet() (whether the key is set, ignoring flag's default).

Fixes #276
Fixes #580

(+ added some tests to enforce this behavior)

@benoitmasson benoitmasson force-pushed the isset-flags branch 2 times, most recently from 2350a42 to 66c1dea Compare April 17, 2017 17:20
@zyuyou zyuyou mentioned this pull request May 15, 2017
Closed
@deanishe deanishe mentioned this pull request Jun 19, 2017
Open
andrewseidl pushed a commit to andrewseidl/viper that referenced this pull request Jul 24, 2017
@benoitmasson @andrewseidl
Tests : added tests to TestIsSet()
ad5beb5 
Added tests for:
- nested elements
- environment values
- flags (currently fails => IsSet() always returns true,
                            due to the default value of the flag)

Cherry picked from spf13#331

(cherry picked from commit ee80f47)
andrewseidl pushed a commit to andrewseidl/viper that referenced this pull request Jul 24, 2017
@benoitmasson @andrewseidl
Fix : find() looks for flag's default values only when requested.
7ac7c4d 
Default value should be looked for by Get(), but not by IsSet().

This logic should remain inside find(), to make sure that shadowing
of keys is handled properly.

Cherry picked from spf13#331

(cherry picked from commit 66c1dea)
@benoitmasson
Copy link
Contributor Author

Just rebased on master, maybe this could be merged sometime?

This solves a small, but real issue.

@sean-
Copy link

sean- commented Feb 20, 2018 •
edited
Loading

@benoitmasson This PR doesn't address types where the zero value is a non-nil value. For example, with string types in StringP(), the default value must be an empty string, but you can't compare an empty string with nil, which limits the value of this PR. Something that incorporates flag's defaultIsZeroValue() is probably necessary to make this PR generally useful. That said, I burned a pile of time trying to figure out that this was broken, so thank you very much for this issue and PR.

@benoitmasson
Copy link
Contributor Author

@sean- Not sure I get your point… IsSet is used to tell whether a config key is defined anywhere (other than flag default, which was not the case before this PR), so comparing the value to defaultZeroValue() does not make sense…

nil is the default zero value of a map[string]interface{} value, which is why values are matched against it to check for existence.

However I may have misunderstood your remark, so feel free to send a test which fails and I will try to fix it.

losinggeneration added a commit to losinggeneration/viper that referenced this pull request Apr 23, 2018
@losinggeneration
Allow getting Sub from a prefix of pflags key(s)
b7a4909 
* It seems common enough that flags may be defined from PFlags be
  retrieved as a sub Viper similar to getting it from Yaml or other
  configuration structures. This allows flags to be bound one-to-one
  with how a config file may be structured.
* This is partially based on spf13#331 for the find boolean flag
* fixes spf13#368
@losinggeneration losinggeneration mentioned this pull request Apr 23, 2018
Open
@berniedurfee-ge
Copy link

Is this waiting on something specific?

@tsuna
Copy link

tsuna commented Nov 15, 2018

Can we get this merged please?

@hacdias
Copy link

hacdias commented Jan 8, 2019

@bep @spf13 what's the state of this PR? This would actually be really useful.

@fastcat fastcat mentioned this pull request Oct 18, 2019
Closed
@hairyhenderson
Copy link

There's a merge conflict, but otherwise this seems ready to merge. Would be awesome to see some movement here!

@hairyhenderson
Copy link

/cc @sagikazarmark - PTAL 🙂

@benoitmasson
Added tests to TestIsSet()
eafbc11 
Added tests for:
- nested elements
- environment values
- flags (currently fails => IsSet() always returns true,
                            due to the default value of the flag)
@benoitmasson
find() looks for flag's default values only when requested.
f9d0975 
Default value should be looked for by Get(), but not by IsSet().

This logic should remain inside find(), to make sure that shadowing
of keys is handled properly.

Fixes Issue spf13#276.
@benoitmasson
Copy link
Contributor Author

Just rebased, it all looks good.

Things have changed since my last update, but this PR is quite light and merging should not be an issue.

@sagikazarmark
Copy link
Collaborator

Thanks for the PR and the ping. I'll review this PR shortly.

@sagikazarmark sagikazarmark added this to the 1.6.0 milestone Nov 28, 2019
sagikazarmark
sagikazarmark approved these changes Nov 29, 2019
View reviewed changes
Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @benoitmasson !

@sagikazarmark sagikazarmark merged commit 9e353e3 into spf13:master Nov 29, 2019
@fastcat fastcat mentioned this pull request Dec 19, 2019
Closed
@ajeetdsouza ajeetdsouza mentioned this pull request Nov 23, 2020
Merged
losinggeneration added a commit to losinggeneration/viper that referenced this pull request Dec 19, 2020
@losinggeneration
Allow getting Sub from a prefix of pflags key(s)
d57958a 
* It seems common enough that flags may be defined from PFlags be
  retrieved as a sub Viper similar to getting it from Yaml or other
  configuration structures. This allows flags to be bound one-to-one
  with how a config file may be structured.
* This is partially based on spf13#331 for the find boolean flag
* fixes spf13#368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagikazarmark sagikazarmark sagikazarmark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.6.0
7 participants
@benoitmasson @sean- @berniedurfee-ge @tsuna @hacdias @hairyhenderson @sagikazarmark