Skip to content

Fix ordering in plyr compats... #3327

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 4 commits into from
May 20, 2019
Merged

Conversation

thomasp85
Copy link
Member

Fixes #3313 and #3315

This fixes the differences in ordering between plyr and the replacement functions. Some notes

  • the plyr ordering in ddply() is based on a pretty weird heuristic but I think I've nailed it
  • the difference in join_keys() is the result of differences in rbind_dfs() and plyr::rbind.fill(). I honestly think the current ggplot2 approach is more correct, but I've made Date handling even with plyr to unbreak revdeps... If that fixes everything we should leave it at that instead of pursuing other parity with other weird vector formats

I probably don't have a head for more work this week, so bear with me if there are any problems with the PR

@thomasp85 thomasp85 added this to the ggplot2 3.2.0 milestone May 14, 2019
@thomasp85 thomasp85 requested a review from clauswilke May 14, 2019 09:09
@yutannihilation
Copy link
Member

It seems this test fails:

test_that("if an aes is mapped to a function that returns NULL, it is removed", {
df <- data_frame(x = 1:10)
null <- function(...) NULL
p <- cdata(ggplot(df, aes(x, null())))
expect_identical(names(p[[1]]), c("PANEL", "x", "group"))
})

Error:

names(p[[1]]) not identical to c("PANEL", "x", "group").
2/3 mismatches
x[1]: "x"
y[1]: "PANEL"

x[2]: "PANEL"
y[2]: "x"

This ordering was tweaked in #3013, so I guess you just need to revert this.

https://github.com/tidyverse/ggplot2/pull/3013/files#diff-12165842aeed0c5cb8c55e833132b58c

R/compat-plyr.R Outdated
@@ -310,6 +314,7 @@ rbind_dfs <- function(dfs) {
attributes(out) <- list(class = "data.frame", names = names(out), row.names = .set_row_names(total))
out
}
date_origin <- Sys.Date() - unclass(Sys.Date())
Copy link
Member

Choose a reason for hiding this comment

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

Is this the kind of global assignment that should be moved to .onLoad()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yeah. Good catch 🙂

@thomasp85 thomasp85 changed the base branch from master to v3.2.0-rc May 20, 2019 20:52
@thomasp85 thomasp85 merged commit ff268a5 into tidyverse:v3.2.0-rc May 20, 2019
@lock
Copy link

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

3 participants