-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: format error messages better (MINOR) #3233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we have two different output errors (JSON and non-JSON). I wonder why we don't keep one output for all errors? If we use JSON, shouldn't we always display JSON error messages so it is consistent?
|
||
// Then: | ||
final String output = terminal.getOutputString(); | ||
if (console.getOutputFormat() == OutputFormat.TABULAR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be an assertion? What if we have a bug where the output format is not TABULAR? This test will never fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON is for the REST API, tabular is what the CLI displays - I think we need both. This test doesn't make sense for the JSON case because we don't do anything special for it (it's testing formatting in the CLI).
w.r.t the assertion, this test class runs all the tests first in JSON, then in TABULAR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I didn't notice this test is parameterized with JSON or TABULAR.
|
||
// Then: | ||
final String output = terminal.getOutputString(); | ||
if (console.getOutputFormat() == OutputFormat.TABULAR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as the previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Do some pretty printing and wrapping to make sure that error messages aren't an eye sore.
Testing done
Before:
After:
Reviewer checklist