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

StringSlice CLI should replace default values #98

Closed
wants to merge 1 commit into from
Closed

StringSlice CLI should replace default values #98

wants to merge 1 commit into from

Conversation

edoardottt
Copy link
Contributor

@edoardottt edoardottt commented Nov 12, 2022

I can confirm PortVarP() is affected as well:

[...]
func main() {
	testOptions := &Options{}
	flagSet := goflags.NewFlagSet()
	flagSet.CreateGroup("info", "Info",
		flagSet.PortVarP(&testOptions.Port, "port", "p", []string{"1"}, "port test"),
	)
	if err := flagSet.Parse(); err != nil {
		log.Fatal(err)
	}
	fmt.Println(testOptions.Port)
}
go run port-flag.go -p 2
(1,2)

Honestly I don't like this solution, it breaks the concept of having the StringSlice exactly equal as []string (And so reflect.Type).
That said, personal considerations:

  • This solution is backward-compatible (in my opinion the most desired property).

  • (flagSet *FlagSet) Parse() calls flagSet.CommandLine.Parse(os.Args[1:]) (ref: https://github.com/projectdiscovery/goflags/blob/main/goflags.go#L96) and it's not possible to override the second method. In that method the way used to set a flag value is to call Set() and (as far as I know) there is no way to distinguish between user-inputted values and default ones. So there is no way to override the default value out of using the Set() method.

  • Each input flag should have the notion of default value, that's why I've thought to change StringSlice to a struct holding the Default indicator.

  • Maybe another solution could be also to change optionMap to be accessible from outside (OptionMap) and insert the boolean value in that struct.

TBH I don't have a clear abstract view of the code. I'm open to discuss, get suggestions and change completely the solution.

This PR closes #95.

@Mzack9999
Copy link
Member

@edoardottt I've tried a different approach to preserve the StringSlice alias type of []string at #102 by using global maps

@edoardottt
Copy link
Contributor Author

@edoardottt I've tried a different approach to preserve the StringSlice alias type of []string at #102 by using global maps

Amazing !!!
I think this PR can be closed now :)

@ehsandeep ehsandeep closed this Dec 15, 2022
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.

StringSlice CLI should replace default values
3 participants