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

change from os.Args scan to flags #301

Closed
CommoDor64 opened this issue Apr 25, 2020 · 7 comments
Closed

change from os.Args scan to flags #301

CommoDor64 opened this issue Apr 25, 2020 · 7 comments
Assignees

Comments

@CommoDor64
Copy link

Current state

Command line flags are being scanned explicitly, this reduces readability, can introduce bugs and make future changes more difficult

Proposal

Delegating this responsibility to https://golang.org/pkg/flag/, as it is 1st party, idiomatic and very easy to use with little chances to introduce any issues.

It will also allow to delegate all flag parsing to a dedicate file or package

Example

app.go

func isPrefork() bool {
	for _, v := range os.Args[1:] {
		if v == "-prefork" {
			return true
		}
	}
	return false
}

turns to

var isPrefork bool
func init() {
	flag.boolVar(&isPrefork, "prefork", 1234, "enable multiple goroutines to listen to a socket")
}
@CommoDor64
Copy link
Author

Not a bug, but not a feature either.

@thomasvvugt
Copy link
Contributor

Hi @CommoDor64 , thanks for creating this issue!
I also prefer using Go's default flag package to parse any program arguments.
However, I doubt if @Fenny had any design choices for using os.Args back when he created the application.
If you like, you can submit a pull request for this change.

If you have any questions, please let me know!

@CommoDor64
Copy link
Author

Sounds great.
I will work on it, is it possible to assign it to me?

@thomasvvugt
Copy link
Contributor

Great, I assigned you to this issue as well.
Thanks for your commitment so far!

@Fenny
Copy link
Member

Fenny commented Apr 25, 2020

@CommoDor64 thanks for your commitment!

The reason we switched to reading the os.Args was because of #159 it created conflicts with other libraries who use the same arguments. flag.Parse also crashed our benchmarks running on docker. Fasthttp had the same issues when used with different libraries and they also switched to os.Args prefork.go#L76

But if you can find a solution to make it compatible with other libraries who also use flag.Parse() your more than welcome to create a PR!

@CommoDor64
Copy link
Author

I suspected there was a valid reason for it.
But since there are only two flags so far, I think we can close it for now if you want (:

@Fenny
Copy link
Member

Fenny commented Apr 25, 2020

@CommoDor64 thanks for your feedback, I do agree that using the default functions is the best practice. We might come back to this in the future 👍

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

No branches or pull requests

5 participants