Skip to content

Port mappings to tidy eval #2467

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Mar 22, 2018
Merged

Port mappings to tidy eval #2467

merged 19 commits into from
Mar 22, 2018

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Mar 1, 2018

No description provided.

@lionel-
Copy link
Member Author

lionel- commented Mar 1, 2018

@thomasp85 is there any ggplot extension out there that makes assumptions about the structure of params$facets, i.e. that it's a quoted object from the plyr package?

@thomasp85
Copy link
Member

Maybe...

Good news is that if there are, they are all in one of my packages with a very high probability - I'll make sure to take care of these...

@lionel- lionel- force-pushed the tidyeval branch 3 times, most recently from f2f3b66 to 095a987 Compare March 22, 2018 11:40
@lionel- lionel- changed the title WIP: Port to tidy eval Port mappings to tidy eval Mar 22, 2018
@lionel- lionel- requested a review from hadley March 22, 2018 11:42
@lionel- lionel- force-pushed the tidyeval branch 2 times, most recently from 9b8a85d to 30f366d Compare March 22, 2018 12:22
hadley
hadley reviewed Mar 22, 2018
View reviewed changes
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Looks good - I didn't read the code too carefully as I assume it works.

Where/when do you want to do the NEWS bullet? I think it might also be worthwhile to soft-deprecate aes_() and aes_string() and link them back to aes() with a suggestion to use quasiquotation.

DESCRIPTION Outdated
hadley/svglite,
jimhester/withr,
tidyverse/rlang
lionel-/rlang@1def2e2cffaaa
Copy link
Member

Choose a reason for hiding this comment

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

I assume there will be an rlang PR soon?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is already one: r-lib/rlang#471

@lionel-
Copy link
Member Author

lionel- commented Mar 22, 2018

I didn't read the code too carefully as I assume it works.

I couldn't run the doppelganger tests because I don't have the right deps on my machine though :/. The default legends are not deparsed correctly (you see the ~ of quosures). I'm going to fix this before merging, but it'd be nice to run the visual tests properly before release. Maybe it's time to finally fix vdiffr.

@lionel- lionel- merged commit cab8a54 into master Mar 22, 2018
@lionel- lionel- deleted the tidyeval branch March 22, 2018 16:54
@ruaridhw ruaridhw mentioned this pull request May 2, 2018
Closed
@lock
Copy link

lock bot commented Sep 18, 2018

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 Sep 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hadley hadley hadley left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lionel- @thomasp85 @hadley