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

Add test for using stage() with a mapping specified for start only #6131

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

yjunechoe
Copy link
Contributor

This PR adds tests to codify the behavior of aes(stage(x)) and its variants working equivalently to aes(x).

I had been under the impression that stage(x) will remain invalid since #4890 was closed without a follow-up (and with a general agreement that this is an odd thing to do), but that issue seems to have been resolved a while back as part of a larger PR in #5409. I assume that the currently working behavior of stage(x) is intended and desirable, and I would personally like for this to be guaranteed as a stable behavior (or at least, for there to be tests to detect its unintended regression).

Hence, the PR adds three tests of equivalence between aes(x) and:

  1. aes(stage(x))
  2. aes(stage(start = x))
  3. aes(stage(start = x, after_stat = NULL, after_scale = NULL))

I saw some prior discussion on the significance of resolving an unnamed value passed to start, so the test includes both (1) and (2). I added (3) just for completeness, but can be removed if this is a weird thing to test (perhaps more meaningful is testing whether (3) works when other arguments are dynamically evaluated to NULL, but I digress).


In case stage(x) wasn't intended and the question of its desirability is still up in the air, I want to advocate for this with a pedagogical use-case: the practical equivalence between aes(x) and aes(stage(x)) makes the introduction of more advanced strategies only one-step removed and thus easier to teach/learn, e.g., aes(fill = stage(x, after_scale = darken(fill, .5)))

@yjunechoe
Copy link
Contributor Author

If this PR makes sense, I'd also like to clean up this comment about how "there should always be two arguments to stage()" :)

ggplot2/R/aes-evaluation.R

Lines 334 to 335 in 4fbc857

# Prefer stat mapping if present, otherwise original mapping (fallback to
# scale mapping) but there should always be two arguments to stage()

@teunbrand
Copy link
Collaborator

Thanks for the PR, June! I agree that stage() working with just the one argument should be equivalent to plain evaluation.

the practical equivalence between aes(x) and aes(stage(x)) makes the introduction of more advanced strategies only one-step removed and thus easier to teach/learn

This might be nice to include in ?aes_eval as well, but this is totally optional.

I'd also like to clean up this comment

This makes sense to me.

@yjunechoe
Copy link
Contributor Author

This might be nice to include in ?aes_eval as well, but this is totally optional.

Will think a bit about this!

@yjunechoe
Copy link
Contributor Author

I added a bit to the "Complex staging" section to spell out this equivalence (of ex: x <-> stage(start = x)). Plus some unsolicited changes to naming of headers (I thought it'd be nice to have the argument param name "start" be present in the "Stage 1" header) and intro paragraph about stage() (which reads to me a bit abrupt, but I recognize that this is function docs not a vignette, so I can roll it back if it's getting too long)

@teunbrand
Copy link
Collaborator

I like it :) Do you think this is good to go or do you envision more edits?

@yjunechoe
Copy link
Contributor Author

That's it! Nothing else in my mind for the scope of this PR

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Looks great, thanks June for the contribution!

@teunbrand teunbrand merged commit ddd207e into tidyverse:main Oct 11, 2024
11 checks passed
@yjunechoe yjunechoe deleted the test-stage-mapping-at-start-only branch October 11, 2024 14:43
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.

2 participants