-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Reporting] Refactor error messages with human-friendly version of message #136501
[Reporting] Refactor error messages with human-friendly version of message #136501
Conversation
…riendly version of errors
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
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.
Some small text changes and a couple questions.
When we write error messages, it's helpful to let the users know what to do to solve the issue.
/** | ||
* Return a message describing the error that is human friendly | ||
*/ | ||
humanFriendlyMessage?(): string; |
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 am just curious why you decided not to go with the built-in toString()
method? That seems like it can very well fit into our use case.
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.
Good question!
In most cases on the server-side we will be using the default toString
behaviour. But when storing this as a message on the Reporting document (where it is seen by users) sometimes we want to have a special human-friendlier version of the stringified error. Does that make sense? Is there another way to achieve this? Perhaps toString('humanFriendly')
?
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.
Thanks, @jloleysens. That makes total sense now 👍
No need to overcomplicate this. The current solution is clear enough.
@elasticmachine merge upstream |
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 👍
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Adds a new method to
ReportingError
to retrieve the human-friendly version of an error message. Currently the code for doing this is a bit convoluted and this refactor attempts to make this functionality more explicit and simpler.