Skip to content

Ensure that we don't try and assign scale to NULL #2600

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
May 16, 2018
Merged

Conversation

hadley
Copy link
Member

@hadley hadley commented May 15, 2018

Fixes #2599

@karawoo any ideas why this behaviour changed?

@hadley hadley requested a review from karawoo May 15, 2018 17:51
@hadley hadley changed the base branch from breaking-changes to master May 15, 2018 17:52
@karawoo
Copy link
Member

karawoo commented May 15, 2018

It looks like the change happened with the port to tidyeval, so maybe @lionel- can weigh in too. This specific error first occurs with 1f39e13. From the first pass at tidyeval commit until then there's a different error:

ggplot(mtcars, aes(mpg, wt)) + 
  geom_point(aes(shape = NULL))
#> Error in if (length(ans) == 0L || as.character(ans[[1L]])[1L] == "~") { : 
#>   missing value where TRUE/FALSE needed

Before all the tidyeval changes, find_scale() was not being called for the shape aesthetic at all.

@hadley
Copy link
Member Author

hadley commented May 15, 2018

I wonder if it's 1f39e13#diff-e01fbacefeedb8a626f9a0ec103d0e12L277

@lionel-
Copy link
Member

lionel- commented May 16, 2018

@hadley NULLs are stored the same way in both dev and CRAN versions:

str(aes(shape = NULL))
#> List of 1
#>  $ shape: NULL

So it seems like the commit you mention is not the cause.

@lionel-
Copy link
Member

lionel- commented May 16, 2018

Also this gives the same results:

x <- aes(shape = NULL)
x[["shape"]] <- NULL
str(x)
#> Named list()

x <- aes(shape = NULL)
x["shape"] <- NULL
str(x)
#> Named list()

x <- aes(shape = NULL)
x$shape <- NULL
str(x)
#> Named list()

@lionel-
Copy link
Member

lionel- commented May 16, 2018

The issue was that plyr::tryapply() strips NULL elements automatically. Compacting aesthetics after evaluation should fix the issue, cf last commit.

@hadley
Copy link
Member Author

hadley commented May 16, 2018

Ah I knew tryapply() was there for some reason!! (I'll try and tweak the unit tests if I have time)

@hadley hadley merged commit bcc3ae5 into master May 16, 2018
@hadley hadley deleted the null-aesthetic branch May 16, 2018 20:19
@lock
Copy link

lock bot commented Nov 12, 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 Nov 12, 2018
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.

3 participants