Skip to content

Take ownership of the mutate() step in group_by() #6139

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

Closed
lionel- opened this issue Dec 21, 2021 · 4 comments · Fixed by #6425
Closed

Take ownership of the mutate() step in group_by() #6139

lionel- opened this issue Dec 21, 2021 · 4 comments · Fixed by #6425
Labels
feature a feature request or enhancement rlang 🔬
Milestone

Comments

@lionel-
Copy link
Member

lionel- commented Dec 21, 2021

To simplify the error message:

mtcars %>%
  group_by(cyl = 1 + "") %>%
  mutate(new = 1 + "")
#> Error in `group_by()` at dplyr/R/mutate.R:156:2:
#> ! Problem adding computed columns.
#> Caused by error in `mutate()`:
#> ! Problem while computing `cyl = 1 + ""`.
#> Caused by error in `1 + ""`:
#> ! non-numeric argument to binary operator
@lionel- lionel- self-assigned this Dec 21, 2021
@hadley hadley added feature a feature request or enhancement rlang 🔬 labels Apr 16, 2022
@hadley
Copy link
Member

hadley commented Jul 22, 2022

Is there some simple way to do this? Because if it's easy count() should also take responsibility for group_by(), as in #6182.

@lionel-
Copy link
Member Author

lionel- commented Jul 23, 2022

To keep the dplyr API user-oriented, we didn't add call arguments to the generics or data frame methods, instead we used a sentinel approach. It works by setting dplyr_local_error_call() before calling other dplyr verbs, then they should borrow the call automatically.

This is still experimental but is used in some places, e.g. in the slice() derivatives like slice_head().

@lionel- lionel- removed their assignment Jul 23, 2022
@hadley
Copy link
Member

hadley commented Jul 23, 2022

Hmmm, adding dplyr_local_error_call() at the obvious place in count.data.frame() doesn't work, and add_computed_columns() already has what looks like an existing attempt to solve this problem, so I think probably someone else will need to take a look at this.

@lionel-
Copy link
Member Author

lionel- commented Jul 24, 2022

Sounds good I'll take a look after the workshop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement rlang 🔬
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants