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.

Sign up for GitHub

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

Jump to bottom

boomer.ignore.inside #95

Closed
wants to merge 5 commits into from
Closed

boomer.ignore.inside #95

wants to merge 5 commits into from

Conversation

moodymudskipper
Copy link
Owner

Closes #93

@krlmlr how does it look ? Note the difference between the magrittr and base pipe here.

It is possible in principle to boom only selected arguments, by manipulating the promise evaluation environment with rlang magic, but then I'm not sure what would the API look like.

library(dplyr, w = F)
library(boomer)

options("boomer.ignore.inside" = list(dplyr::mutate))

tibble(a = 1:3) |>
  mutate(.by = a, b = a + 1) |>
  identity() |>
  boom()
#> 💣 identity(mutate(tibble(a = 1:3), .by = a, b = a + 1)) 
#> · 💣 💥 mutate(tibble(a = 1:3), .by = a, b = a + 1) 
#> · # A tibble: 3 × 2
#> ·       a     b
#> ·   <int> <dbl>
#> · 1     1     2
#> · 2     2     3
#> · 3     3     4
#> · 
#> 💥 identity(mutate(tibble(a = 1:3), .by = a, b = a + 1)) 
#> # A tibble: 3 × 2
#>       a     b
#>   <int> <dbl>
#> 1     1     2
#> 2     2     3
#> 3     3     4
#> # A tibble: 3 × 2
#>       a     b
#>   <int> <dbl>
#> 1     1     2
#> 2     2     3
#> 3     3     4

tibble(a = 1:3) %>%
  mutate(.by = a, b = a + 1) %>%
  identity() %>%
  boom()
#> 💣 tibble(a = 1:3) %>%... 
#> · 💣 identity(.) 
#> · · · 💣 tibble(a = 1:3) 
#> · · · · 💣 💥 1:3 
#> · · · · [1] 1 2 3
#> · · · · 
#> · · · 💥 tibble(a = 1:3) 
#> · · · # A tibble: 3 × 1
#> · · ·       a
#> · · ·   <int>
#> · · · 1     1
#> · · · 2     2
#> · · · 3     3
#> · · · 
#> · · 💣 💥 mutate(., .by = a, b = a + 1) 
#> · · # A tibble: 3 × 2
#> · ·       a     b
#> · ·   <int> <dbl>
#> · · 1     1     2
#> · · 2     2     3
#> · · 3     3     4
#> · · 
#> · 💥 identity(.) 
#> · # A tibble: 3 × 2
#> ·       a     b
#> ·   <int> <dbl>
#> · 1     1     2
#> · 2     2     3
#> · 3     3     4
#> · 
#> 💥 tibble(a = 1:3) %>%
#>      mutate(.by = a, b = a + 1) %>%
#>      identity() 
#> # A tibble: 3 × 2
#>       a     b
#>   <int> <dbl>
#> 1     1     2
#> 2     2     3
#> 3     3     4
#> # A tibble: 3 × 2
#>       a     b
#>   <int> <dbl>
#> 1     1     2
#> 2     2     3
#> 3     3     4

Created on 2024-01-18 with reprex v2.0.2

@krlmlr
Copy link
Collaborator

krlmlr commented Jan 22, 2024

Thanks. It's mentioned in the NEWS, and it's clear from the code -- but I'm still curious why boomer.ignore needs a character vector and boomer.ignore.inside needs a list of functions. Would it be possible and perhaps cleaner to move the check for "ignore inside" to the same place where the check for "ignore" happens?

@moodymudskipper
Copy link
Owner Author

boomer.ignore was designed so we can ignore common silent primitives, so providing character was enough. For boomer.ignore.inside if I could support "mutate" but I have also to support "dplyr::mutate", values made more sense, but the inconsistency does look bad.
An improvement might be to support both input types for both options, so we're backward compatible and also consistent.
I don't believe I can make the 2 checks closer, the check on boomer.ignore happens as soon as possible and returns early, the other check needs other objects to be built.

@moodymudskipper
Copy link
Owner Author

done elsewhere, the argument is named boomer.ignore_args and it can be either a character vector or a named list of functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krlmlr krlmlr Awaiting requested review from krlmlr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

boomer and grouped operations
2 participants
@moodymudskipper @krlmlr