-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Demote errors in legend modified expressions #6267
base: main
Are you sure you want to change the base?
Demote errors in legend modified expressions #6267
Conversation
Thanks -- this works for my use-case! I think the p <- ggplot(mpg, aes(drv)) +
geom_bar(
aes(fill = after_scale(stop())),
# show.legend = FALSE
) On main:p
#> Error in `geom_bar()`:
#> ! Problem while setting up geom aesthetics.
#> ℹ Error occurred in the 1st layer.
#> Caused by error in `after_scale()`: (plot fails to build) On branch:p
#> Warning: Unable to apply legend modifications.
#> Caused by error in `after_scale()`: May be out of scope and may not need this as part of the solution, but I wonder whether it's worth considering passing some kind of a flag to |
You raise a good point, though IMO the error message is overspecific. If we just say the staged modifications failed, without mentioning legend or layer, we needn't fuss about passing along context. |
modified_aes <- try_fetch( | ||
eval_aesthetics( | ||
substitute_aes(modifiers), data, | ||
mask = list(stage = stage_scaled) | ||
), | ||
error = function(cnd) { | ||
cli::cli_warn("Unable to apply staged modifications.", parent = cnd) | ||
data_frame0() | ||
} |
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 setup means that an issue in one staged expression nullifies all staged expressions, right?
Maybe this is ok as it is not often we have a huge amount of staged expressions in the same layer...
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.
Yes that is right. I think it is already a pretty niche situation when you run into one of these errors in the first place.
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
This PR aims to fix #6264.
When we have modified expressions with e.g.
stage()
that fail to evaluate, we demote the error to a warning and don't apply these expressions.