feat: Display CLI usage on user error #819
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
orpheuslummis
added
the
action/no-benchmark
Codecov Report
@@ Coverage Diff @@
## develop #819 +/- ##
===========================================
- Coverage 59.57% 59.56% -0.02%
===========================================
Files 155 155
Lines 17271 17275 +4
===========================================
Hits 10290 10290
- Misses 6051 6055 +4
Partials 930 930
|
orpheuslummis
changed the title
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jsimnz
requested changes
Sep 19, 2022
cli/cli.go
Outdated
Comment on lines
57
to
64
| for _, cobraError := range cobraErrors { | ||
| if strings.HasPrefix(err.Error(), cobraError) { | ||
| if usageErr := rootCmd.Usage(); usageErr != nil { | ||
| log.FeedbackFatalE(ctx, "error displaying usage help", usageErr) | ||
| } | ||
| } | ||
| } | ||
| log.FeedbackErrorE(ctx, "DefraDB error", err) |
There was a problem hiding this comment.
Suggestion: I'm not a fan of "DefraDB error" as a prefix here.
Additionally, there should be a difference between execution error and usage error. At the moment, it feels centered around usage error.
If it is a usage error, the prefix should indicate so, and should be logged before the Usage Help.
The original line log.FeedbackError(ctx, fmt.Sprintf("%s", err)) is prob helpful, maybe with slight modification for the execution error. It lets the error stand on its own.
There was a problem hiding this comment.
Thanks for the feedback.
FeedbackErrorE is used to support the inclusion of stacktrace if that's desired, and it requires a prefix, and includes the error message (and stacktrace) as KVs. It could be updated if the internal errors pkg would allow access to the stacktrace.
I've updated.
There was a problem hiding this comment.
Not sure why you say it requires a prefix. Do you equate the message param to a prefix? Also, why would you want to have access to the stacktrace?
There was a problem hiding this comment.
By prefix I meant message param, yes.
For consistency, as the rest of logging gives stacktraces on demand. A user might want to obtain stacktraces from the top-level returned error too.
There was a problem hiding this comment.
This can be done already. Just use:
fmt.Printf("%+v", err)to get the stacktrace
There was a problem hiding this comment.
thanks for the info Fred. i knew about it but had forgotten
There was a problem hiding this comment.
updated with Fred's feedback
fredcarle
reviewed
Sep 20, 2022
cli/cli.go
Outdated
Comment on lines
60
to
64
| if cfg.Log.Stacktrace { | ||
| log.FeedbackError(ctx, fmt.Sprintf("%+v", err)) | ||
| } else { | ||
| log.FeedbackError(ctx, err.Error()) | ||
| } |
There was a problem hiding this comment.
question: I'm not sure I understand why you are doing this. Why not keep a message string and let the logger handle displaying the error's stacktrace?
There was a problem hiding this comment.
two currently conflicting goals:
- have consistent log output formats across defradb
- pleasant human experience using the program
for this PR it's best to prioritize have consistent log output formats across defradb, so reverted back.
in upcoming CLI work i'll resolve these currently conflicting goals ...
orpheuslummis
force-pushed
the
orpheus/feat/cli-usage-on-user-error
branch
from
053c9f7
to
3c7e4e2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #669
Description
Displays usage text on 'user error' (failure to summon a valid command).
Given current release timeline, I suggest tests for this are to be included in next release.
Limitations:
DefraDB errorin addition of the error itself because internal logger interface doesn't allow to display directly an error and its stacktrace (when we configured to display stacktrace).I thought that was the least worse way to achieve this UX feature given the options I considered (listed in the issue).
Tasks
How has this been tested?
Manual
Specify the platform(s) on which this was tested: