nathaneastwood / poorman

A poor man's dependency free grammar of data manipulation
https://nathaneastwood.github.io/poorman/
Other
338 stars 15 forks source link

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

Closed etiennebacher closed 2 years ago

etiennebacher commented 2 years ago

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 commented 2 years ago

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).