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

flag: add TextVar to handle types that implement encoding.TextUnmarshaler #45754

Closed
dsnet opened this issue Apr 25, 2021 · 11 comments
Closed

flag: add TextVar to handle types that implement encoding.TextUnmarshaler #45754

dsnet opened this issue Apr 25, 2021 · 11 comments

Comments

@dsnet
Copy link
Member

dsnet commented Apr 25, 2021

The encoding.TextMarshaler and encoding.TextUnmarshaler interfaces are the de-facto way for a type to self-report that it can serialize to/from some humanly readable text format.

The easiest ways to tie the flag package to encoding.TextUnmarshaler is to:

  1. use flag.Func to declare an anonymous function that calls some unmarshal function under the hood. This approach takes >3 lines, but unfortunately does not display the default value in the usage documentation.
  2. declare a type that implements flag.Value and then use flag.Var. This approach requires declaring 1 types and 2 methods and is generally more than 10 lines of cost.

Both ways have detriments (either too long or drops default values being printed).

I propose adding:

// TextVar defines a flag with a specified name, default value, and usage string.
// The argument p points to a variable in which to store the value of the flag.
// The flag accepts a value according to encoding.TextUnmarshaler.UnmarshalText.
// The default value type must be the same as the pointed at variable type.
func TextVar(p encoding.TextUnmarshaler, name string, value encoding.TextMarshaler, usage string)

along with the equivalent method on the FlagSet type.

Example usage would be:

var t time.Time
flag.TextVar(&t, "start_time", time.Now(), "Time to start processing at.")

or

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

Of note, this is approach only requires a single line (other than the variable declaration) and works with any type that implements encoding.TextMarshaler and encoding.TextUnmarshaler.

There is a type safety loss where the p argument and value argument may not be the same concrete type. Generics could resolve this problem.


Within the standard library, there are 5 types that implement the interfaces:

Each one could conceivably be useful to apply with the flag package.

According to the latest version of all modules, there are ~14k types that implement encoding.TextUnmarshaler out of ~8.2M types total. While this only accounts 0.2% of all types, it's still a non-trivial number of implementations.

@gopherbot
Copy link

Change https://golang.org/cl/313329 mentions this issue: flag: add TextVar function

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 25, 2021
@mvdan
Copy link
Member

mvdan commented Apr 26, 2021

This is clever. I've often found people reaching for third-party flag libraries just for the sake of having more types built-in and not having to implement flag.Value manually.

This could also nudge more libraries towards implementing TextMarshaler and TextUnmarshaler, and I think that's a good thing. The encoding package is rarely used directly, and I think some Go developers aren't even well aware of this "text encoding" interface. Those libraries would rarely go out of their way to implement flag.Value directly, but implementing the two text methods is much more reasonable and reusable.

@icholy
Copy link

icholy commented Apr 27, 2021

I could have used this today when adding a flag to control the logrus.Level of a service.

@rsc
Copy link
Contributor

rsc commented May 5, 2021

/cc @robpike

@rsc
Copy link
Contributor

rsc commented May 5, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@robpike
Copy link
Contributor

robpike commented May 18, 2021

Seems reasonable to me. Might be able to forestall some future requests.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) May 19, 2021
@rsc
Copy link
Contributor

rsc commented May 19, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) May 26, 2021
@rsc
Copy link
Contributor

rsc commented May 26, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: flag: add TextVar to handle types that implement encoding.TextUnmarshaler flag: add TextVar to handle types that implement encoding.TextUnmarshaler May 26, 2021
@rsc rsc modified the milestones: Proposal, Backlog May 26, 2021
@earthboundkid
Copy link
Contributor

earthboundkid commented Jul 1, 2021

flag.Var doesn't take a default value. Can't the TextUnmarshaller passed in be the default value, e.g.

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

You could do an optional interface check for TextMarshaller, String, and then fallback to fmt.Sprint when printing the default.

@dsnet
Copy link
Member Author

dsnet commented Jul 3, 2021

All the other functions that have Var as a suffix (e.g., BoolVar, DurationVar, IntVar, etc.) take in a default value as an argument. I had TextVar take in a default argument for consistency with those functions.

There is a readability advantage to keeping the default value with the flag declaration when multiple flags are declared:

var (
	enable bool
	period time.Duration
	ipaddr net.IP
	start  time.Time
	seed   int64
	email  string
)
flag.BoolVar(&enable, "enable", true, "should we run this task?")
flag.DurationVar(&period, "period", time.Minute, "how often to refresh the task?")
flag.TextVar(&ipaddr, "ipaddr", net.IPv4(192, 168, 0, 1), "what server to connect to?")
flag.TextVar(&start, "start", time.Now(), "when should we start processing?")
flag.IntVar(&seed, "seed", 388342, "what seed to use for pseudo-randomization?")
flag.StringVar(&email, "email", "admin@domain.com", "what email should we send results to?")

It would be reasonable if defaults were set first on the values before passing to Var-suffixed functions:

var (
	enable = true
	period = time.Minute
	ipaddr = net.IPv4(192, 168, 0, 1)
	start  = time.Now()
	seed   = int64(388342)
	email  = "admin@domain.com"
)
flag.BoolVar(&enable, "enable", "should we run this task?")
flag.DurationVar(&period, "period", "how often to refresh the task?")
flag.TextVar(&ipaddr, "ipaddr", "what server to connect to?")
flag.TextVar(&start, "start", "when should we start processing?")
flag.IntVar(&seed, "seed", "what seed to use for pseudo-randomization?")
flag.StringVar(&email, "email", "what email should we send results to?")

However, that's not how the API works today.

@earthboundkid
Copy link
Contributor

Makes sense.

@golang golang locked and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants