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: clean up error message #26822

Closed
vrothberg opened this issue Aug 6, 2018 · 19 comments
Closed

flag: clean up error message #26822

vrothberg opened this issue Aug 6, 2018 · 19 comments

Comments

@vrothberg
Copy link

@vrothberg vrothberg commented Aug 6, 2018

What version of Go are you using (go version)?

go version go1.9.7 linux/amd64 (but it affects basically all versions of go).

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GOHOSTARCH="amd64"
GOHOSTOS="linux"

What did you do?

Provide a non-numerical input for an integer flag yields the following error:

invalid value "donut" for flag -cpu-quota: strconv.ParseInt: parsing "donut": invalid syntax

What did you expect to see?

--cpuquota is expecting an integer

Something more user-friendly. Currently, the error message must be interpreted by the user and should guide users a bit more in a direction to solve the issue. See containers/buildah#673 as an example.

What did you see instead?

invalid value "donut" for flag -cpu-quota: strconv.ParseInt: parsing "donut": invalid syntax

Proposal

I want to suggest two options:

  1. Update the error messages (e.g., for integer flags https://golang.org/src/flag/flag.go?s=8120:8324#L912).
  2. Expose public error types (e.g., ErrInvalidValue and ErrInvalidBoolen) that can be used by users of the flag package (e.g., urfave/cli).
@robpike
Copy link
Contributor

@robpike robpike commented Aug 6, 2018

The error message can be improved, but changing the public API requires going through the proposal process. Updating the issue.

@robpike robpike added Proposal and removed NeedsFix labels Aug 6, 2018
@robpike robpike changed the title flag: parsing: improve error {messages, handling} proposal: improve errors in flag package, export error types Aug 6, 2018
@vrothberg
Copy link
Author

@vrothberg vrothberg commented Aug 7, 2018

Thanks a lot, @robpike, for the quick response. Much appreciated!

Improving the error messages sounds good to me.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 20, 2018

What's missing from this proposal is the proposal of what the new API looks like.

The current five errors are:

return false, f.failf("bad flag syntax: %s", s)
return false, f.failf("invalid boolean value %q for -%s: %v", value, name, err)
return false, f.failf("invalid boolean flag %s: %v", name, err)
return false, f.failf("flag needs an argument: -%s", name)
return false, f.failf("invalid value %q for flag -%s: %v", value, name, err)

As far as exported API, can we make do with a single new flag.Error struct, or do these need to be multiple error types? Maybe something like:

type Error struct {
    Flag *Flag
    Value string
    Implicit bool
    Err error 
}

is at least a starting point for a discussion.

@rsc rsc changed the title proposal: improve errors in flag package, export error types proposal: flag: export parse error type Aug 20, 2018
@vrothberg
Copy link
Author

@vrothberg vrothberg commented Aug 21, 2018

@rsc, thanks a lot for looking into the issue.

As far as exported API, can we make do with a single new flag.Error struct, or do these need to be multiple error types?

[updated]:
There are actually cases where it's really helpful to distinguish the errors (e.g.,
urfave/cli#758 (comment)). Having different types of errors would avoid string compares.

It may be the lack of coffee, but the meaning of the Implicit bool field is not obvious to me. @rsc, could you explain your intention behind that field?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 21, 2018

I'm not sure what Implicit is for but don't worry about it. Construct a type that lets you recreate the existing errors and let's see how it looks. Thanks.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 19, 2018

Implicit was to distinguish between -x=true and -x for a boolean x. (is "Value" implicit?)

Does anyone want to sketch an implementation and based on that send back a proposed API?

@rsc rsc added the help wanted label Oct 3, 2018
@iand
Copy link
Contributor

@iand iand commented Oct 9, 2018

I've started work on an implementation to explore this.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 10, 2018

Change https://golang.org/cl/141017 mentions this issue: flag: return typed errors when parsing

@iand
Copy link
Contributor

@iand iand commented Oct 10, 2018

CL 141017 implements typed parse errors for discussion in this issue.

I felt that there were two distinct categories of errors that would be useful to distinguish: undefined/unknown flags and syntax errors for known flags.

I represented these with two new error types:

// UndefinedError is the custom error type returned when Parse encounters
// a flag that is not defined in the FlagSet.
type UndefinedError struct {
	Name string
}

// SyntaxError is the custom error type returned when Parse encounters
// a flag value or name that cannot be parsed.
type SyntaxError struct {
	Flag  *Flag // Flag may be nil if the flag name could not be determined
	Value string
	Err   error
}

The current error strings are mapped to the two new types and I added a test to check compatibility for people who may be currently matching string prefixes.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 10, 2018

/cc @robpike

@robpike
Copy link
Contributor

@robpike robpike commented Oct 11, 2018

I am not convinced we need special types here. I don't see how a program needs to behave in an important different way based on the particular way a flag fails. The current state is very simple. Why not just improve the error messages?

@iand
Copy link
Contributor

@iand iand commented Oct 12, 2018

@robpike changing the error messages would break existing users who are already string matching on the error text. Hyrum's rule in play. The design I put in the CL would let people cleanly detect the type of error and also gives them the core information to build their own error message. It preserves existing messages for the string matchers like the poster of comment above.

@robpike
Copy link
Contributor

@robpike robpike commented Oct 12, 2018

I agree with that being the reason, my question was whether the reasons apply to the flag package, whose errors do not require recovery or special handling. If the command line is wrong, the program does not continue. Even subcommands do not continue. The user reads the help message and tries again.

@iand
Copy link
Contributor

@iand iand commented Oct 12, 2018

The particular issue that led to this proposal is using FlagSet to enable the overlay of some additional parsing rules:

Only split a given string (e.g., "-abc") into short options (e.g., "-a",
"-b", "-c") if all those are flags. To further avoid mistakenly
transform common arguments, catch "flag provided but not defined" errors
to iteratively transform short options.
@robpike
Copy link
Contributor

@robpike robpike commented Oct 12, 2018

That is an argument for a different, external, flags package, one with different rules for parsing flags, not an argument for changing this one.

@iand
Copy link
Contributor

@iand iand commented Oct 13, 2018

I don't think so. The inclusion of the FlagSet type is a signal to users that the flag package is intended to be used for creating alternate flag arrangements such as for subcommands. A FlagSet is also not tied to the current program's command line. It could be used to check arguments being provided to another program. Providing better information about failures seems reasonable.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 16, 2018

If you want to look at the registered flags and make a new flag parser, do that. Don't try to parse the parser errors - just do a fresh parse. See rsc.io/getopt for an example.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 16, 2018

Let's fix the error messages to be cleaner and then worry about whether to export any new API separately. Assigning to Rob and repurposing this issue for the error messages alone.

@rsc rsc changed the title proposal: flag: export parse error type flag: clean up error message Oct 16, 2018
@rsc rsc modified the milestones: Unplanned, Go1.12 Oct 16, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 19, 2018

Change https://golang.org/cl/143257 mentions this issue: flag: return a consistent parse error if the flag value is invalid

@gopherbot gopherbot closed this in ae0c435 Oct 19, 2018
@golang golang locked and limited conversation to collaborators Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.