Made error messages in CLI more user friendly #222
Merged
Conversation
This file contains hidden or 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
Difference in output when info flag is given compared to when debug flag is givenDEBUG FLAGINFO FLAG |
fhembroff
commented
Feb 6, 2023
| } else if (!"".equals(exception.getMessage())) { | ||
| err(exception.getMessage()); | ||
| } else if ("".equals(message)) { | ||
| err("An unknown error has occurred. Consider rerunning with --debug"); |
There was a problem hiding this comment.
Should I be advising users to rerun with --debug?
Also, I will change --debug to the DEBUG constant once #221 is merged.
There was a problem hiding this comment.
Should I be advising users to rerun with
--debug?
That make sense, although we don't have too many LOG.debug statements. But we have enough.
coverbeck
reviewed
Feb 7, 2023
| if (LOG.isInfoEnabled()) { | ||
| LOG.error(arg); | ||
| } else { | ||
| System.err.println(ERROR_MESSAGE_START + arg); |
There was a problem hiding this comment.
Use io.dockstore.client.cli.ArgumentUtility#err for consistency.
There was a problem hiding this comment.
This is where err is defined. Would it better to call this method, printError and define err to be System.err.println?
| } else if (!"".equals(exception.getMessage())) { | ||
| err(exception.getMessage()); | ||
| } else if ("".equals(message)) { | ||
| err("An unknown error has occurred. Consider rerunning with --debug"); |
There was a problem hiding this comment.
Should I be advising users to rerun with
--debug?
That make sense, although we don't have too many LOG.debug statements. But we have enough.
denis-yuen
reviewed
Feb 7, 2023
…clean-up-error-messages
coverbeck
approved these changes
Feb 7, 2023
| if (LOG.isInfoEnabled()) { | ||
| LOG.error(arg); | ||
| } else { | ||
| System.err.println(ERROR_MESSAGE_START + arg); |
Codecov ReportBase: 69.39% // Head: 69.65% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #222 +/- ##
=============================================
+ Coverage 69.39% 69.65% +0.26%
- Complexity 1050 1069 +19
=============================================
Files 47 47
Lines 6034 6041 +7
Branches 789 792 +3
=============================================
+ Hits 4187 4208 +21
+ Misses 1515 1510 -5
+ Partials 332 323 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I think this suggestion from SonarCloud would just complicate things and make them less readable in this situation. |
denis-yuen
approved these changes
Feb 7, 2023
svonworl
approved these changes
Feb 7, 2023
| } | ||
|
|
||
| private static void errFormatted(String format, Object... args) { | ||
| LOG.error((String.format(format, args))); | ||
| if (LOG.isInfoEnabled()) { | ||
| LOG.error(String.format(format, args)); |
There was a problem hiding this comment.
We might be able to eliminate some duplicate code by replacing the body of this function with a call to err(String.format(format, args))
There was a problem hiding this comment.
Thanks, I have now done this.
|
|
||
| if (LOG.isInfoEnabled()) { | ||
| err(ExceptionUtils.getStackTrace(exception)); | ||
| } else if (exception.getMessage() != null && !"".equals(exception.getMessage())) { |
There was a problem hiding this comment.
StringUtils.isNotEmpty would make this clearer: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isNotEmpty-java.lang.CharSequence-
There was a problem hiding this comment.
Thanks, I have now done this.
kathy-t
approved these changes
Feb 8, 2023
|
Kudos, SonarCloud Quality Gate passed! |
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.
Description
This PR improves the readability of error messages given in the CLI. If the
--debugor--infois not given then the CLI will only display (inSystem.err)Where
<ERROR_MESSAGE>is:messageif it is given when exceptionMessage is called"An unknown error has occurred. Consider rerunning with --debug"if the above two are not availableI am using
LOG.isInfoEnabled()to determine whether I should the "nice" output, or the output with the stack traces. This is because it will allow developers to use the--infoflag instead of the--debugflag to see the output. As the--debugflag creates a lot of output. I have made a comment showing the difference in output when--debugis used, as compared to--info.Examples of error messages
Before
After
Before
After
Review Instructions
Verify that error messages are given to users in a user-friendly way.
Ensure that error messages are still given to users and that the error messages are helpful.
Issue
dockstore/dockstore#5223
DOCK-2275
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
./mvnw clean install