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

basicflag provider support skips already set items #230

Closed
wants to merge 5 commits into from

Conversation

beihai0xff
Copy link

basicflag provider will skips over any flag values that have default values set, saving unnecessary processing for already set items.
Tests are modified to include the basicflag module.

beihai0xff and others added 4 commits August 19, 2023 00:09
The new stdflag provider reads commandline parameters as configuration maps. The provider also skips over any flag values that have default values set, saving unnecessary processing for already set items. Tests are modified to include the stdflag module.
@knadh
Copy link
Owner

knadh commented Sep 12, 2023

Hi @beihai0xff. This breaks backwards compatibility with changes to Provider() and ProviderWithValue().

@beihai0xff
Copy link
Author

Hi @beihai0xff. This breaks backwards compatibility with changes to Provider() and ProviderWithValue().

Can I add some new functions to see if the flags defined have been set from other providers? Like the code below:

func ProviderKo(f *flag.FlagSet, delim string, ko KoanfIntf) *Pflag {}
func ProviderKoWithValue(f *flag.FlagSet, delim string, ko KoanfIntf, cb CallBack) *Pflag {}

@knadh knadh self-assigned this Sep 15, 2023
@knadh
Copy link
Owner

knadh commented Sep 23, 2023

Thinking about this, perhaps it's better to not integrate these changes into the core as they are opinionated? It's all easily pluggable anyway.

@beihai0xff
Copy link
Author

Thinking about this, perhaps it's better to not integrate these changes into the core as they are opinionated? It's all easily pluggable anyway.

I add an option argument opts as type Option func(*Pflag) that modifies the Pflag. If you do not want to auto merge the flags defined have been set from other providers, no opts argument will be passed, but if you want to enable auto merge, should add basicflag.WithEnableMerge
Now You can call Provider() like below:

basicflag.Provider(bf, ".")
basicflag.Provider(fs, ".", basicflag.WithEnableMerge(k))
basicflag.Provider(fs, ".", basicflag.WithEnableMerge(k),
		basicflag.WithCallBack(func(key string, value flag.Value) (string, any) {
			return key, value.(flag.Getter).Get()
		}))

Take the long view, we can add more option function for extensibility

@knadh
Copy link
Owner

knadh commented Sep 27, 2023

Thanks, but I think it's best not to change the current implementation for now. If there are more requests for customizing basicflag's behaviour in the future, this can be considered. For now, you can continue to maintain your version of the provider in case anyone else also wants to use it. All providers are pluggable in koanf precisely so that it's easy to maintain different opinionated providers that can be plugged easily.

Thanks again.

@knadh knadh closed this Sep 27, 2023
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.

2 participants