Skip to content

Add evaluating environment to tidy eval call. Closes #2835 #2836

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

Merged
merged 2 commits into from
Aug 20, 2018

Conversation

clauswilke
Copy link
Member

This PR adds the calling environment to the transformation evaluation in the secondary axis. This closes #2835.

@lionel-
Copy link
Member

lionel- commented Aug 18, 2018

Can you add a unit test please? This would be a good start:

n <- 100
p <- ggplot() + scale_y_continuous(sec_axis(trans = ~ . / n))
expect_error(regexp = NA, ggplot_build(p))

No need to wrap in local() because all testthat blocks are local. It'd be even better to test that the axis has been transformed correctly.

Edit: Or would it be good practice to wrap in local() anyway, so you can evaluate line by line interactively?

@lionel-
Copy link
Member

lionel- commented Aug 18, 2018

@hadley I think we should provide a way of supporting quasiquotation for this type of transformation. There's a bug in @IndrajeetPatil's function: whenever the data frame contains a column total_obs, the column has precedence over the environment. Quasiquotation would help solving the precedence.

We could support quosures:

sec_axis(trans = quo(. / !!total_obs))

But that feels like a strange UI. It might be better to provide a formula constructor in rlang:

sec_axis(trans = f(. / !!total_obs))

Which could also be useful for statistical models.

@clauswilke
Copy link
Member Author

@lionel- Thanks, I will add the test case you suggest and also think about making one that tests scaling. Does the proposed fix to the bug look correct to you?

@lionel-
Copy link
Member

lionel- commented Aug 18, 2018

Does the proposed fix to the bug look correct to you?

Yup, but if we'd like to support quosures it'll need a slightly different approach. If we go for a formula constructor with qq support then there's no further change needed.

@clauswilke
Copy link
Member Author

clauswilke commented Aug 18, 2018

There was a unit test already that I could adapt with very minor change.

@lionel- Does this change look good to you or do we need another one?

@dpseidel I think you wrote that test function. Could you look over my modification also?

@lionel-
Copy link
Member

lionel- commented Aug 18, 2018

Looks good!

@dpseidel
Copy link
Collaborator

Yep! Your modification shouldn't change the original intent, looks good to me.

@clauswilke
Copy link
Member Author

Great, thanks! If somebody can approve I'll merge.

@clauswilke clauswilke merged commit 242a7db into tidyverse:master Aug 20, 2018
@clauswilke clauswilke deleted the issue-2835-sec_axis branch August 20, 2018 13:39
@lock
Copy link

lock bot commented Feb 16, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Feb 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issues with plotting secondary axis
4 participants