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
0xmichalis opened this issue Apr 24, 2021 · 7 comments
Closed

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

0xmichalis opened this issue Apr 24, 2021 · 7 comments

Comments

@0xmichalis
Copy link
Contributor

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 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 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 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 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.

@earthboundkid
Copy link
Contributor

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 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

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

@golang golang locked and limited conversation to collaborators May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants