fix: permit empty string for ErrPrefix#2392
Conversation
Currently, when a developer sets `cmd.SetErrPrefix("")`, the system incorrectly assumes the prefix was left unset due to `""` being the default zero-value for a string, and falls back to prepending `"Error: "` and a trailing space. This makes it impossible to output pure JSON errors because of the leading space injected by `c.PrintErrln`.
This PR introduces an unexported `errPrefixSet` boolean to cleanly demarcate when the user has explicitly `SetErrPrefix`.
* Modified `ErrPrefix()` getter to honor the marker without breaking the public `Command` struct API.
* Added a conditional downstream print block inside `ExecuteC` to prevent `fmt.Sprintln` from injecting a leading space when the prefix evaluates to empty.
* Added `TestRootErrPrefixEmpty` to `command_test.go` to explicitly prevent regressions.
* Existing unit tests pass.
Fixes spf13#2226
|
|
|
Do we need a way to unset the error prefix now that using |
|
This makes sense to me but I believe this has backwards-compatibility implications. Existing programs might be using It wouldn’t be pretty but we could have a function |
|
Good catch on the backwards-compatibility issue! You're totally right that existing programs might be relying on I've pushed an update that reverts Let me know what you think of this approach! |
| @@ -377,6 +379,12 @@ | |||
| c.errPrefix = s | |||
There was a problem hiding this comment.
If this function is called with an argument "" it should unset the error prefix which should cause ErrPrefix() to check if the parent has an error prefix.
This won't happen if SetErrPrefixEmpty(true) has been called before.
I think we also need to call c.errPrefixEmpty = false in this function to truly unset the error prefix
| // SetErrPrefixEmpty sets whether the error message prefix should be empty. | ||
| // If set to true, this suppresses the default "Error: " prefix. | ||
| func (c *Command) SetErrPrefixEmpty(empty bool) { | ||
| c.errPrefixEmpty = empty |
There was a problem hiding this comment.
Same idea that if SetErrPrefix("hello") was called before, this function should undo that, or else the error prefix will not actually show as empty.
So we should set c.errPrefix = "" here I think
| c.versionTemplate = tmpl(s) | ||
| } | ||
|
|
||
| // SetErrPrefix sets error message prefix to be used. Application can use it to set custom prefix. |
There was a problem hiding this comment.
Can you add a comment saying that "to unset the error prefix, this function should be called with the empty string. To prevent an error prefix from being used, the function SetErrPrefixEmpty(true) should be used"
| } | ||
|
|
||
| // SetErrPrefixEmpty sets whether the error message prefix should be empty. | ||
| // If set to true, this suppresses the default "Error: " prefix. |
There was a problem hiding this comment.
Please a comment along the lines of to unset the error prefix (so the parent chain can be checked), please use SetErrPrefix("")
|
@marckhouzam Thanks for the great review! I really appreciate the time you've taken to look into this and provide such helpful feedback. I've just pushed a commit that addresses your points:
I'm thrilled to be able to contribute back to a project I use every day. Let me know if there's anything else needed! |
3 participants
Currently, when a developer sets
cmd.SetErrPrefix(""), the system incorrectly assumes the prefix was left unset due to""being the default zero-value for a string, and falls back to prepending"Error: "and a trailing space. This makes it impossible to output pure JSON errors because of the leading space injected byc.PrintErrln.This PR introduces an unexported
errPrefixSetboolean to cleanly demarcate when the user has explicitlySetErrPrefix.ErrPrefix()getter to honor the marker without breaking the publicCommandstruct API.ExecuteCto preventfmt.Sprintlnfrom injecting a leading space when the prefix evaluates to empty.TestRootErrPrefixEmptytocommand_test.goto explicitly prevent regressions.Fixes #2226