Report the source of a value when we cannot parse it

[mostynb]

What type of PR is this?

• feature

What this PR does / why we need it

If you allow a flag to be set from environment variables or files and a parse error occurs from one of them, it is very useful for the error message to mention where the value came from.

Without this, it can be difficult to notice an error caused by an unexpected environment variable being set.

Implements #1167.

The existing function flagFromEnvOrFile takes a slice of strings (environment variable names) and a file path, and checks each to see if they exist. If they do, then it returns the value and a bool which is true if a value was found.

This patch modifies it to also return a string description of where the value was found. Callers have been updated to use this extra value when reporting errors.

Which issue(s) this PR fixes:

Implements #1167.

Special notes for your reviewer:

I haven't quite figured out what should be done in flag_path.go and flag_string.go yet- any advice?

Testing

Existing tests have been updated to expect the new error strings.

Release Notes

NONE

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[mostynb]

Bump.

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[asahasrabuddhe]

Instead of returning a source from the function flagFromEnvOrFile why don't you directly return an error ? The function can return a value and error instead of value and boolean. This error can then directly returned by the Apply function.

[mostynb]

Instead of returning a source from the function flagFromEnvOrFile why don't you directly return an error ? The function can return a value and error instead of value and boolean. This error can then directly returned by the Apply function.

(Force-pushed a rebase onto master, which resolved a basic merge conflict. The test_docs CI failure looks unrelated, and is also failing on other PRs.)

@asahasrabuddhe flagFromEnvOrFile just returns the first raw/unparsed string value from multiple possible sources that exist- the only possible error it can return with the current input type signature is "no value found in any of the possible sources", which is why it currently returns a bool instead of an error.

At the moment, the parse error checking is performed by the caller, eg flag_bool.go calls flagFromEnvOrFile and tries to parse the value as a bool, flag_float64.go tries to parse the value as a float64 and so on. If we want flagFromEnvOrFile to return a parse/invalid format error, it would need to take an empty interface argument, and have big type switch for every possible format (and hope that we don't forget any types). This doesn't sound nice to me- is this what you want?

[rliebz]

I agree with the assessment that the boolean is more appropriate to return in this case. We're not interested in reporting an error if we fail to parse a value—we're interested in reporting an error if the value that we parsed in the helper function is invalid.

The signature is a bit awkward as is, but I don't think there's a great way around it. If we're looking for something more well-structured, we could go with a custom type:

type source struct {
  Type string // this could also be custom type, if we're really into custom types
  Name string
}

func (s source) String() string {
  // `environment variable "FOO"` or `file "bar.txt"`
  return fmt.Sprintf("%s %q", s.Type, s.Name)
}

func flagFromEnvOrFile(envVars []string, filePath string) (string, source, bool) {
  // ...
    return val, source{Type: "environment variable", Name: envVar}, true
  // ...
    return string(data), source{Type: "file", Name: filePath}, true
  // ...
  return "", source{}, false
}

It feels like a fair bit of ceremony, but it seems more obvious on a read why the signature looks the way it does. Just throwing the idea out there, not sure it's necessarily better.

[mostynb]

I agree with the assessment that the boolean is more appropriate to return in this case. We're not interested in reporting an error if we fail to parse a value—we're interested in reporting an error if the value that we parsed in the helper function is invalid.

The signature is a bit awkward as is, but I don't think there's a great way around it. If we're looking for something more well-structured, we could go with a custom type:
...
It feels like a fair bit of ceremony, but it seems more obvious on a read why the signature looks the way it does. Just throwing the idea out there, not sure it's necessarily better.

I'm not sure what benefit that would give us. In the end all we want is a string description of where the value came from for an error message.

I added a function comment in the latest fixup, maybe that improves the readability while keeping the simpler form- what do you think?

[rliebz]

I'm not sure what benefit that would give us.

Types are documentation. But variable names are documentation, and documentation is documentation. I'm happy with the current iteration.

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[mostynb]

Ping.

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale]

Closing this as it has become stale.

The base branch was changed.