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

Report the source of a value when we cannot parse it #1168

Merged
merged 2 commits into from May 22, 2022

Conversation

mostynb
Copy link
Contributor

@mostynb mostynb commented Jul 30, 2020

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

@mostynb mostynb requested a review from a team as a code owner Jul 30, 2020
@mostynb mostynb requested review from rliebz and asahasrabuddhe and removed request for a team Jul 30, 2020
@stale
Copy link

stale bot commented Oct 28, 2020

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 stale bot added the status/stale stale due to the age of it's last update label Oct 28, 2020
@mostynb
Copy link
Contributor Author

mostynb commented Oct 28, 2020

Bump.

@stale
Copy link

stale bot commented Oct 28, 2020

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 stale bot removed the status/stale stale due to the age of it's last update label Oct 28, 2020
Copy link
Member

@asahasrabuddhe asahasrabuddhe left a comment

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.

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 urfave#1167.
@mostynb
Copy link
Contributor Author

mostynb commented Nov 3, 2020

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?

@mostynb mostynb force-pushed the report_source_of_parse_errors branch from 1d60a91 to 500d6b0 Compare Nov 3, 2020
flag.go Outdated Show resolved Hide resolved
flag_path.go Outdated Show resolved Hide resolved
flag_string.go Outdated Show resolved Hide resolved
@rliebz
Copy link
Member

rliebz commented Nov 6, 2020

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.

move bool to the end of the return arguments

remove "from " prefix in the source/fromWhere description

remove TODO notes from functions that don't currently perform error checking
@mostynb
Copy link
Contributor Author

mostynb commented Nov 7, 2020

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
Copy link
Member

rliebz commented Nov 7, 2020

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.

rliebz
rliebz previously approved these changes Nov 7, 2020
@mostynb mostynb requested a review from asahasrabuddhe Nov 16, 2020
@stale
Copy link

stale bot commented Feb 15, 2021

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 stale bot added the status/stale stale due to the age of it's last update label Feb 15, 2021
@mostynb
Copy link
Contributor Author

mostynb commented Feb 15, 2021

Ping.

@stale
Copy link

stale bot commented Feb 15, 2021

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 stale bot removed the status/stale stale due to the age of it's last update label Feb 15, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

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 stale bot added the status/stale stale due to the age of it's last update label Jun 2, 2021
@stale
Copy link

stale bot commented Jul 4, 2021

Closing this as it has become stale.

@stale stale bot closed this Jul 4, 2021
@meatballhat meatballhat reopened this Apr 19, 2022
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 and removed status/stale stale due to the age of it's last update labels Apr 21, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 21, 2022
@meatballhat meatballhat changed the base branch from master to main Apr 21, 2022
@meatballhat meatballhat dismissed rliebz’s stale review Apr 21, 2022

The base branch was changed.

@meatballhat meatballhat added the status/superseded has been superseded label May 8, 2022
@meatballhat meatballhat merged commit 826b3ed into urfave:main May 22, 2022
9 of 10 checks passed
@mostynb mostynb deleted the report_source_of_parse_errors branch May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/feature describes a code enhancement / feature request status/superseded has been superseded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants