Fix getFlagType error message

[wellitonscheer]

Closes #461

[CLAassistant]

All committers have signed the CLA.

[tomasaschan]

I think this is maybe a confusion around the English language, rather than about the flags themselves. (And English is not my first language either...)

But you said it yourself in #461:

"if in the code i change the 'lines' to something else that is not a defined flag"

This is exactly what the error message is telling you. It is a message to the programmer of the application that there is a bug (where a flag is accessed using a dynamic lookup, but it is not defined). It's not a message to the user (e.g. that the command was given incorrect flags). So

That said, I don't think it's important to it keep as-is; the Get method on FlagSet does not exist in stdlib's flag, so we can do what we want with it without affecting being a "drop-in replacement". It seems to me, though, that if you replace this instance of this error, that removes the only place where it's used, which means it should probably also be cleaned out from the enum and from the case construct that renders a message.

[thaJeztah]

Overall looks good, but left a comment about removing the const.

I agree, I don't think we need to keep it specifically, and it seems fine to me to use "flag "--foo" does not exist"; at least I think it's clearer.

Out of scope for this PR, but perhaps it would even be useful to indicate that we tried to lookup a flag named X with type FOO.

I think all these need some work; there's some issues with these errors overall (I'll post below).

[thaJeztah]

(all out of scope for this PR; let me post here, but I can create a ticket for it as well)

there's some issues with these errors overall (I'll post below).

Heh; went slightly down the rabbit-hole;

I was curious about the const / iota used for these to see if they carried any meaning, but it looks they were added in 8d77158 and just enumerated each of the error formats used, creating a const for each message; but some of them were only used once;

But there's no consistency whatsover in the error messages; different wording, different formatting (no quote, double quotes, colon / no colon; some expect a "bare" name, others a normalized name (including -)

I even wonder if it's worth trying to shoehorn them all into some "smart" formatting code or if we should just keep the NotExistError error-type (useful as sentinel error?) with the name and/or short name in it (as those have methods to return them) but otherwise allow the caller to format the message as they please.

At least there's various issues with the implementation that should be looked at;

• The zero value is not really usable
• Methods defined with pointer-receivers, which ... produce unexpected results
• The name must be specified either with or without -, depending on the type
• Unknown types panic (not sure if that's really expected)
• flagUnknownShorthandFlagMessage with an empty name will panic

Here's a basic test showing some of those issues;

func TestErrors(t *testing.T) {
	t.Run("zero value", func(t *testing.T) {
		myErr := &NotExistError{}
		t.Log("Error: ", myErr.Error())
		t.Log("GetSpecifiedName: ", myErr.GetSpecifiedName())
		t.Log("GetSpecifiedShortnames: ", myErr.GetSpecifiedShortnames())
	})
	t.Run("invalid value", func(t *testing.T) {
		t.Log(&NotExistError{messageType: 99})
	})
	t.Run("invalid value (no pointer)", func(t *testing.T) {
		t.Log(NotExistError{messageType: 99})
	})
	t.Run("error types", func(t *testing.T) {
		var names = []string{"", "a", "aa", "-a", "--a", "-aa", "--aa"}
		for mt := 0; mt <= 4; mt++ {
			for _, name := range names {
				t.Log(&NotExistError{
					name:                name,
					specifiedShorthands: name,
					messageType:         notExistErrorMessageType(mt),
				})
			}
		}
	})
}

== RUN   TestErrors
=== RUN   TestErrors/zero_value
    errors_test.go:72: Error:  flag "" does not exist
    errors_test.go:73: GetSpecifiedName:  
    errors_test.go:74: GetSpecifiedShortnames:  
--- PASS: TestErrors/zero_value (0.00s)
=== RUN   TestErrors/invalid_value
    errors_test.go:77: %!v(PANIC=Error method: unknown flagNotExistErrorMessageType: 99)
--- PASS: TestErrors/invalid_value (0.00s)
=== RUN   TestErrors/invalid_value_(no_pointer)
    errors_test.go:80: {  99}
--- PASS: TestErrors/invalid_value_(no_pointer) (0.00s)
=== RUN   TestErrors/error_types
    errors_test.go:86: flag "" does not exist
    errors_test.go:86: flag "a" does not exist
    errors_test.go:86: flag "aa" does not exist
    errors_test.go:86: flag "-a" does not exist
    errors_test.go:86: flag "--a" does not exist
    errors_test.go:86: flag "-aa" does not exist
    errors_test.go:86: flag "--aa" does not exist
    errors_test.go:86: flag accessed but not defined: 
    errors_test.go:86: flag accessed but not defined: a
    errors_test.go:86: flag accessed but not defined: aa
    errors_test.go:86: flag accessed but not defined: -a
    errors_test.go:86: flag accessed but not defined: --a
    errors_test.go:86: flag accessed but not defined: -aa
    errors_test.go:86: flag accessed but not defined: --aa
    errors_test.go:86: no such flag -
    errors_test.go:86: no such flag -a
    errors_test.go:86: no such flag -aa
    errors_test.go:86: no such flag --a
    errors_test.go:86: no such flag ---a
    errors_test.go:86: no such flag --aa
    errors_test.go:86: no such flag ---aa
    errors_test.go:86: unknown flag: --
    errors_test.go:86: unknown flag: --a
    errors_test.go:86: unknown flag: --aa
    errors_test.go:86: unknown flag: ---a
    errors_test.go:86: unknown flag: ----a
    errors_test.go:86: unknown flag: ---aa
    errors_test.go:86: unknown flag: ----aa
    errors_test.go:86: %!v(PANIC=Error method: runtime error: index out of range [0] with length 0)
    errors_test.go:86: unknown shorthand flag: 'a' in -a
    errors_test.go:86: unknown shorthand flag: 'a' in -aa
    errors_test.go:86: unknown shorthand flag: '-' in --a
    errors_test.go:86: unknown shorthand flag: '-' in ---a
    errors_test.go:86: unknown shorthand flag: '-' in --aa
    errors_test.go:86: unknown shorthand flag: '-' in ---aa
--- PASS: TestErrors/error_types (0.00s)
--- PASS: TestErrors (0.00s)
PASS

[tomasaschan]

@thaJeztah Thanks for digging into this more thoroughly! I think moving your analysis into an issue (and punting the actual decision on what to do about it) makes a lot of sense.

@wellitonscheer I think this looks good now and will approve and merge, assuming all tests pass. Thank you for your contribution!

Has been addressed!