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

metaprogramming - substitute #873

Closed
Polkas opened this issue Dec 15, 2021 · 6 comments · Fixed by #876
Closed

metaprogramming - substitute #873

Polkas opened this issue Dec 15, 2021 · 6 comments · Fixed by #876

Comments

@Polkas
Copy link
Contributor

Polkas commented Dec 15, 2021

I will be glad if you consider to support one of the metaprogramming scenario.
Unfortunately I find out quite dangerous side effect of styler styling.

Example where we will expect the styler to NOT add brackets in %>% pipeline:

styler::style_text('substitute(DATA %>% FUN_EXPR, env = list(FUN_EXPR = call("FUN")))')
# substitute(DATA %>% FUN_EXPR(), env = list(FUN_EXPR = call("FUN")))
# whereas should be neutral
# substitute(DATA %>% FUN_EXPR, env = list(FUN_EXPR = call("FUN")))

The usage of bquote might be a solution here nevertheless substitute is way more efficient:

> microbenchmark::microbenchmark(bquote = {bquote(x <- .(letters))}, substitute = {substitute(x <- ll, list(ll = letters))})
#Unit: nanoseconds
#       expr  min   lq    mean median     uq   max neval cld
#     bquote 4387 4510 4809.71   4592 4674.0 24313   100   b
# substitute  164  246  281.67    246  266.5  2378   100  a 
@lorenzwalthert
Copy link
Collaborator

Thanks @Polkas. Are you suggesting that the rule that adds () should not be applied inside substitute()? This might be feasible.

Also, this substitution is using base R. {styler} is a tidyverse focussed formatter and there are other (and probably better) tools to do this, namely tidy evaluation. Not sure the problem would occur there too.

@Polkas
Copy link
Contributor Author

Polkas commented Dec 15, 2021

Thank you very much for a quick response.
Yes, a rule that adds () should not be applied inside substitute(). I will be glad if it could be applied. The code design is not assumed to use tidy rules. Thanks

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 15, 2021

Ok, just for the sake of completeness, can you provide a reprex where the current {styler} behavior fails? I.e. changes the result of running the code?

@Polkas
Copy link
Contributor Author

Polkas commented Dec 15, 2021

Reprex, which is showing that finally we get FUN_head()() (variable styled_expr) instead of FUN_head() (variable expr):

library(magrittr)
FUN_head <- function(x) head(x)
expr <- substitute(airquality %>% FUN_EXPR, env = list(FUN_EXPR = call("FUN_head")))
print(expr)
#> airquality %>% FUN_head()
eval(expr)
#>   Ozone Solar.R Wind Temp Month Day
#> 1    41     190  7.4   67     5   1
#> 2    36     118  8.0   72     5   2
#> 3    12     149 12.6   74     5   3
#> 4    18     313 11.5   62     5   4
#> 5    NA      NA 14.3   56     5   5
#> 6    28      NA 14.9   66     5   6
styled_expr_str <- styler::style_text('substitute(airquality %>% FUN_EXPR, env = list(FUN_EXPR = call("FUN_head")))')
# double eval as frist one return only result of substitute
styled_expr <- eval(str2lang(styled_expr_str))
print(styled_expr)
#> airquality %>% FUN_head()()
eval(styled_expr)
#> Error in head(x): argument "x" is missing, with no default

Created on 2021-12-15 by the reprex package (v2.0.1)

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 16, 2021

So, we can solve this when substitute() is explicitly called, but it it is a placeholder itself, we can't track the reference. I.e. in

placeholder <- substitute
placeholder(x %>% y, env =list(y = call("head")))

y -> y() with applying {styler}. Don't think this can be solved -.- We had to disable the rule completely within function calls.

@Polkas
Copy link
Contributor Author

Polkas commented Dec 17, 2021

I understand this is only a regex rule so my expectancy was not so high. I think an update of the case where the usage of substitute is direct will be more than enough. Thank you once more for the support in this issue.

lorenzwalthert added a commit that referenced this issue Dec 17, 2021
- Don't add `()` within substitute piped calls (#873).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants