Skip to content
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

Mask stage() at the expression level #6110

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Sep 17, 2024

This PR aims to fix #6104.
It is mutually exclusive with #6108 and #6107.

Briefly, this PR always masks the syntactic functions after_stat(), after_scale() and stage().
That way, you needn't prefix it with ggplot2:: if you don't have the namespace attached.

# Note: this PR is locally installed for this example
ggplot2::ggplot(ggplot2::mpg, ggplot2::aes(displ, hwy)) +
  ggplot2::geom_point(
    ggplot2::aes(fill = stage(drv, after_scale = scales::alpha(fill, 0.3))),
    shape = 21
  )

And users cannot override these functions, at least when used inside aes().

library(ggplot2)

stage <- function(...) stop("error")

ggplot(mpg, aes(displ, hwy)) +
  geom_point(
    aes(fill = stage(drv, after_scale = alpha(fill, 0.3))),
    shape = 21
  )

Created on 2024-09-17 with reprex v2.1.1

@teunbrand
Copy link
Collaborator Author

teunbrand commented Sep 23, 2024

Alright as a hail mary, I've changed from wrestling with data masks to changing the calls in the unevaluated aesthetics.
Thus, stage() now acts as any other function in that stage() doesn't work unless ggplot2 is loaded and ggplot2::stage() behaves as expected.
The only slightly insidious behaviour is that if another package, let's say {foobar} defines a stage() function, then this gets supplanted by ggplot2's functions, even if using foobar::stage() verbatim.

# Note: this PR is locally installed for this example
ggplot2::ggplot(ggplot2::mpg, ggplot2::aes(displ, hwy)) +
  ggplot2::geom_point(
    ggplot2::aes(fill = ggplot2::stage(drv, after_scale = scales::alpha(fill, 0.3))),
    shape = 21
  )

# We cannot use naked `stage()` when we haven't loaded ggplot2
ggplot2::ggplot(ggplot2::mpg, ggplot2::aes(displ, hwy)) +
  ggplot2::geom_point(
    ggplot2::aes(fill = stage(drv, after_scale = scales::alpha(fill, 0.3))),
    shape = 21
  )
#> Error in `ggplot2::geom_point()`:
#> ! Problem while computing aesthetics.
#> ℹ Error occurred in the 1st layer.
#> Caused by error in `stage()`:
#> ! could not find function "stage"

Created on 2024-09-23 with reprex v2.1.1

@teunbrand teunbrand changed the title Always use a syntax mask Mask stage() at the expression level Sep 23, 2024
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't suppose there is an elegant way to test this?

@teunbrand
Copy link
Collaborator Author

The masking mechanic is testable, using ggplot2::stage() is testable, but the 'if ggplot2 is not loaded' condition is hard to test.

@thomasp85
Copy link
Member

Well, that is probably sufficient :-)

So please add a test for the namespacing

@teunbrand teunbrand merged commit 4fbc857 into tidyverse:main Sep 23, 2024
13 checks passed
@teunbrand teunbrand deleted the syntax_mask branch September 23, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"stage()" does not work properly when prefixed with "ggplot2::"
2 participants