-
Notifications
You must be signed in to change notification settings - Fork 2.1k
get_layer_data(), get_layer_grob() can use layer names #6724
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
base: main
Are you sure you want to change the base?
Conversation
…to layer-names
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.
Thank you for the PR! I agree that being able to use the name to get layer data/grob would be useful. I think it'd make more sense to merge to name approach in this PR with i. Using vctrs::vec_as_location2() to construct the index would handle both the integer and named cases.
|
Thanks, I didn't know that helper existed! Now the parameter |
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 for these updates, I like the direction of the PR!
Now the parameter i is a bit inconsistent with get_panel_scales() since it indexes into panels instead of layers.
I agree that it is unlikely to be an issue.
| # name falls back to index | ||
| expect_snapshot_error( | ||
| get_layer_data(p, i ="none") | ||
| ) | ||
| expect_snapshot_error( | ||
| get_layer_data(p, i = 4L) | ||
| ) |
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.
It is good that these errors are thrown, but I don't think ggplot2 needs to test for them. They are outside ggplot2's control as they are thrown by the vctrs package. If the message changes, that doesn't imply that ggplot2 needs to change.
| expect_snapshot_error( | ||
| get_layer_grob(p, i ="none") | ||
| ) | ||
| expect_snapshot_error( | ||
| get_layer_grob(p, i = 4L) | ||
| ) |
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.
Likewise, I think we don't need these tests.
| expect_identical( | ||
| get_layer_data(p, i = "bar"), | ||
| get_layer_data(p, i = 2L) | ||
| ) |
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 think the intent of this test is good. However, I think the test would also pass if the index was mismatched because the layer data of both layers would be the same.
A tiny PR that builds upon #5967.
Now the names can be used by
get_layer_data()andget_layer_grob()in place of the layer index.Only scalars are accepted, just like the current behavior.
If a name is supplied, it is used, but the layer must exist.
If both the name and the index are supplied (either explicitly or as a default argument), the name takes precedence.
The names must be unique (this is currently enforced by #5967 — should revisit whether this is no longer the case).
Perhaps it's better to make
iandnamemutually exclusive, likeidandnumberforrecipes::tidy.recipe()?