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

Fix: use NSE for "names_from" and "values_from" in pivot_wider() #103

Merged

Conversation

etiennebacher
Copy link
Contributor

This fixes the NSE problem mentioned in #98 and #101. Using quoted and unquoted names for names_from and values_from in pivot_wider() now give the same output.

suppressPackageStartupMessages({
  library(poorman)
})


##### Example 1

production <- expand.grid(
  product = c("A", "B"),
  country = c("AI", "EI"),
  year = 2000:2014
) %>%
  filter((product == "A" & country == "AI") | product == "B") %>%
  mutate(production = rnorm(nrow(.)))

identical(
  production %>%
    poorman::pivot_wider(
      names_from = c(product, country),
      values_from = production
    ),
  production %>%
    poorman::pivot_wider(
      names_from = c("product", "country"),
      values_from = "production"
    )
)
#> [1] TRUE

##### Example 2

identical(
  tidyr::us_rent_income %>%
    as.data.frame() %>% 
    poorman::pivot_wider(
      names_from = variable,
      values_from = c(estimate, moe)
    ),
  tidyr::us_rent_income %>%
    as.data.frame() %>% 
    poorman::pivot_wider(
      names_from = "variable",
      values_from = c("estimate", "moe")
    )
)
#> [1] TRUE


##### Example 3

identical(
  tidyr::us_rent_income %>%
    as.data.frame() %>% 
    poorman::pivot_wider(
      names_from = variable,
      names_sep = ".",
      values_from = c(estimate, moe)
    ),
  tidyr::us_rent_income %>%
    as.data.frame() %>% 
    poorman::pivot_wider(
      names_from = "variable",
      names_sep = ".",
      values_from = c("estimate", "moe")
    )
)
#> [1] TRUE

Created on 2022-08-02 by the reprex package (v2.0.1)

In #101, you said that poorman is not designed to work with tibbles, and I understand that. However, when I made the examples for this PR, it took me a few minutes to realize that pivot_wider() didn't work at all with tibbles. I guess I'm not the only one who was surprised by this. Do you think a warning should be made when the dataset used is a tibble instead of a dataframe?

@nathaneastwood
Copy link
Owner

Do you think a warning should be made when the dataset used is a tibble instead of a dataframe?

Nope. Honestly poorman shouldn't need to know about the existence of tibbles since it is designed to work with data.frames. It's the same as it shouldn't need to know about the existence of data.tables. If it did, we would need to write S3 methods for each of data.frames, tibbles and data.tables. So it is "out of scope" :) that's not to say that in the future we could do what I describe but it'd be a lot of work (I actually abandoned work on data.table methods a long time ago).

@nathaneastwood nathaneastwood merged commit 0529205 into nathaneastwood:master Aug 2, 2022
@etiennebacher etiennebacher deleted the fix_nse_pivot_wider branch August 2, 2022 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