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

proposal: flag: add support for parsing big numbers #45751

Closed
kargakis opened this issue Apr 24, 2021 · 7 comments
Closed

proposal: flag: add support for parsing big numbers #45751

kargakis opened this issue Apr 24, 2021 · 7 comments
Labels
Milestone

Comments

@kargakis
Copy link
Contributor

@kargakis kargakis commented Apr 24, 2021

It would be nice to add support in the flag package to parse a flag directly to a big integer instead of having to parse as a string first and then try to parse the string as a big int in our code. Something like the following:

var candidate = flag.BigInt("n", "3853882583591558728", "Run integer factorization for this number")

If such a proposal makes sense, we might as well add support for all math/big types (Float, Int, Rat).

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 24, 2021

This would cause the flag package to have a dependency on math/big, which would be a non-trivial dependency to absorb. Also, if flag supports math/big types, what would be the metric that we decide what types it supports? Should it also support time.Time?

As a counter-proposal, perhaps flag should support anything that implements encoding.TextMarshaler and encoding.TextUnmarshaler.

Perhaps, the API should be:

func TextVar(p encoding.TextUnmarshaler, name string, value encoding.TextMarshaler, usage string)

and usage could be like:

var n big.Int
flag.TextVar(&n, "n", big.NewInt(3853882583591558728), "Run integer factorization for this number")

or like:

var t time.Time
flag.TextVar(&t, "start_time", time.Now(), "Start processing logs at this time."

I see several advantages of this approach:

  1. it avoids causing the flag package to depend on other packages just to support a given flag type,
  2. it works for more types than just big.Int, and
  3. it relies on a well defined interface for converting text to structured Go values and vice-versa.

There is some type safety problems with my proposed API, where the p and value arguments are not guaranteed to be the same type or pointers to the same type. This could be fixed with generics.

@dsnet dsnet changed the title flag: add support for parsing big numbers proposal: flag: add support for parsing big numbers Apr 24, 2021
@gopherbot gopherbot added this to the Proposal milestone Apr 24, 2021
@robpike
Copy link
Contributor

@robpike robpike commented Apr 24, 2021

It is so easy to write your own implementation of flag.Value that adding more types of flags to the package requires a very strong justification.

@cespare
Copy link
Contributor

@cespare cespare commented Apr 24, 2021

If it's just a one-off, it can be even shorter using the new flag.Func:

n := big.NewInt(3853882583591558728)
flag.Func("n", "Run integer factorization for this number", func(s string) error {
	if _, ok := n.SetString(s, 10); !ok {
		return fmt.Errorf("bad integer %q", s)
	}
	return nil
})

https://play.golang.org/p/U5v9HWN8zF5

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 25, 2021

A detriment of flag.Func is that it doesn't display the default value, which I personally found to be an unfortunate UX hindrance.

I filed #45754 as a counter proposal based on my comment above.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Apr 25, 2021

FWIW, the original implementation of flag.Func took a default value string, but it was dropped because you can just add “(default x)” to the usage string.

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 26, 2021

An advantage of having the flag package manage printing the default value is that updating the actual default value in the code doesn't accidentally cause the manually written default value to become stale.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 11, 2021

Based on the discussion this proposal will clearly not be accepted, so closing. Please comment if you disagree. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants