-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Chained errors with contextual info in layers #4856
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
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.
This is awesome 😄
Looks great! I notice in these examples that we are missing full stops in error messages, which are recommended by our style guide. Should we take this opportunity to ensure full stops in the messages? Also in dplyr chained errors follow this phrasing:
So maybe this:
Should be:
|
This is great! One minor comment: There is a bit of a naming inconsistency between "stat" and "statistic", sometimes you use one, sometimes the other. I think "stat" is used much more widely and so I would consider using it always. There are two cases where you currently write out "statistic", I believe:
I think it would be fine to write instead:
(Don't feel strongly about this either way.) |
This PR improves the error reporting when errors occur during evaluations of a layer. It also adds an improvement to all errors in ggproto methods since the methods are now called by name so we avoid the useless
Error in f()
we get now.The new look of layer errors is:
which is a mouthful but a huge improvement in terms of letting the user understand how their code is wrong
The remaining question is if this error chaining should be expanded to other ggproto objects in play during plot rendering. I feel it is less imperative than the layer errors because facets and coords are singular in the plot and they are thus less likely to cause confusion as to where the error occurred. I'm however open to expanding the
try_fetch
implementations inggplot_build()
to cover it all