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

Open
dsnet opened this issue Apr 25, 2021 · 8 comments
Open

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

dsnet opened this issue Apr 25, 2021 · 8 comments

Comments

@dsnet
Copy link
Member

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

@gopherbot gopherbot commented Apr 25, 2021

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

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

@mvdan 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 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 rsc commented May 5, 2021

/cc @robpike

@rsc
Copy link
Contributor

@rsc 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 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 May 19, 2021
@rsc
Copy link
Contributor

@rsc 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 May 26, 2021
@rsc
Copy link
Contributor

@rsc 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants