-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Duplicated aes warning with multiple modified aesthetics #4707
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
Could you please add a simple unit test that uses |
Sure, are you OK with bumping the {testthat} requirement to >=3.1.2 for this test? |
Yes, that's fine. |
Thanks, I also bumped rlang to >=1.0.0 due to a recently added use of |
tests/testthat/test-aes-calculated.r
Outdated
test_that("staged aesthetics warn appropriately for duplicated names", { | ||
# Test should *not* report `NA` as the duplicated aes (#4707) | ||
df <- data.frame(x = 1, y = 1, lab = "test") | ||
expr <- substitute( |
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.
Since I don't think you're actually doing any substitution, quote()
would be more appropriate here. But I think you could also just do p <- ggplot() + ...
in the expect_snapshot_warning()
.
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 points, I wasn't aware of this, so thanks!
tests/testthat/test-aes-calculated.r
Outdated
) | ||
# One warning in plot code due to evaluation of `aes()` | ||
expect_snapshot_warning(p <- eval(expr)) | ||
# Two warnings in building due to `stage()`/`after_scale()` |
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 only see one warning in the test?
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.
Ah indeed! The second of those warnings only occurred when ggplot_build(p)
was evaluated in the console, due to automatically printing the result. I've now disabled the legend, which caused that extra warning.
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! The last thing is to add a bullet to the top of NEWS.md
. It should briefly describe the change and end with (@yourname, #issuenumber)
.
Thanks! |
This is a simple fix for some warnings that are thrown when multiple modified aesthetics are used.
The warnings could be triggered with the reprex below. With this PR, these warnings won't be triggered.
Effectively, the two chunks of code below take the names of the (modified) aesthetics twice.
ggplot2/R/geom-.r
Line 146 in c89c265
ggplot2/R/aes.r
Lines 168 to 169 in 759c63c
This surprisingly works well when there is only a single modified aesthetic, but not so much with multiple ones.
The following erroneous code (duplicated colour/color):
Used to throw these warnings:
And now throws the following warnings: