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

Reverse direction of -> #409

Open
krlmlr opened this Issue Jul 10, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@krlmlr
Member

krlmlr commented Jul 10, 2018

# Before:
data %>%
  mutate(...) %>%
  select(...) -> result

# After:
result <-
  data %>%
  mutate(...) %>%
  select(...)

Can we do this easily?

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Jul 14, 2018

Not sure, have to check. We put all expressions between pipes on the same level, but the assignment is on a different level:

code <- "
data %>%
mutate(...) %>%
select(...) -> result
"
styler:::create_tree(strsplit(code, "\n", fixed = TRUE))
#>                                                  levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {pos_id})
#> 2   °--expr:  [0/0] {1}                                   
#> 3       ¦--expr:  [0/0] {3}                               
#> 4       ¦   °--SYMBOL_FUNCTION_CALL: c [0/0] {2}          
#> 5       ¦--'(': ( [0/0] {4}                               
#> 6       ¦--expr:  [0/0] {6}                               
#> 7       ¦   °--STR_CONST: "" [0/0] {5}                    
#> 8       ¦--',': , [0/1] {7}                               
#> 9       ¦--expr:  [0/0] {9}                               
#> 10      ¦   °--STR_CONST: "data [0/0] {8}                 
#> 11      ¦--',': , [0/1] {10}                              
#> 12      ¦--expr:  [0/0] {12}                              
#> 13      ¦   °--STR_CONST: "muta [0/0] {11}                
#> 14      ¦--',': , [0/1] {13}                              
#> 15      ¦--expr:  [0/0] {15}                              
#> 16      ¦   °--STR_CONST: "sele [0/0] {14}                
#> 17      °--')': ) [0/0] {16}

pd <- styler:::compute_parse_data_nested(code)
pd$child[[1]]
#> # A tibble: 3 x 15
#>      id pos_id line1  col1 line2  col2 parent token   terminal text  short
#>   <int>  <int> <int> <int> <int> <int>  <int> <chr>   <lgl>    <chr> <chr>
#> 1    29      2     2     1     4    11     33 expr    FALSE    ""    ""   
#> 2    28     22     4    13     4    14     33 RIGHT_… TRUE     ->    ->   
#> 3    32     24     4    16     4    21     33 expr    FALSE    ""    ""   
#> # ... with 4 more variables: token_before <chr>, token_after <chr>,
#> #   internal <lgl>, child <list>
pd$child[[1]]$child[[1]]
#> # A tibble: 5 x 15
#>      id pos_id line1  col1 line2  col2 parent token   terminal text  short
#>   <int>  <int> <int> <int> <int> <int>  <int> <chr>   <lgl>    <chr> <chr>
#> 1     5      5     2     1     2     4     17 expr    FALSE    ""    ""   
#> 2     4      6     2     6     2     8     17 SPECIA… TRUE     %>%   %>%  
#> 3    15      7     3     1     3    11     17 expr    FALSE    ""    ""   
#> 4    16     14     3    13     3    15     29 SPECIA… TRUE     %>%   %>%  
#> 5    27     15     4     1     4    11     29 expr    FALSE    ""    ""   
#> # ... with 4 more variables: token_before <chr>, token_after <chr>,
#> #   internal <lgl>, child <list>

Created on 2018-07-15 by the reprex
package
(v0.2.0).

One hurdle is that every token has to have an pos_id for sorting and I don't remember exactly how that worked. Probably possible but not a low-hanging fruit. Do you see this pattern often in code? Probably if styler could reformat it, people would write code like this more because it's more convenient and they know styler is going to reformat.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Jul 14, 2018

Thanks. If we just reversed the rows of pd$child[[1]] (except id which we'd leave untouched) and changed RIGHT_... to LEFT_...? Do the nests refer to parent IDs in any way?

It's just a use case, I can change it manually but I thought it might be useful.

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Jul 14, 2018

Yes, I thought about not touching pos_id. But this might have unexpected consequences, although I am not aware of any. We might just want to try.

@wlandau

This comment has been minimized.

wlandau commented Aug 18, 2018

FYI: CodeDepends reverses ->. cc @gmbecker.

CodeDepends::getInputs(quote(1 -> x))@code
#> x <- 1
@gmbecker

This comment has been minimized.

gmbecker commented Aug 20, 2018

Would love to take credit for this, but it's the parser doing the work here. There isn't actually a separate -> function/"instruction" within the evaluator:

> parse(text = "5 -> x")
expression(5 -> x)
> parse(text = "5 -> x")[[1]]
x <- 5

See also:

> `<-`
.Primitive("<-")
> `->`
Error: object '->' not found
@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Sep 12, 2018

So could we simply parse / deparse and parse again? Does not seem to have a large speed impact. I.e. styling of this example takes ~100 milliseconds, and we will add 0.5 milliseconds.

code <- "5 -> x; {{{{{{{{x}}}}}}}}"
twice <- quote(styler:::get_parse_data(deparse(parse(text = code)[[1]])))
once <-  quote(styler:::get_parse_data(code))
microbenchmark::microbenchmark(eval(once), eval(twice))
#> Unit: milliseconds
#>         expr      min       lq     mean   median       uq        max neval
#>   eval(once) 1.430916 1.701178 3.506884 2.056408 2.548293 125.718171   100
#>  eval(twice) 1.094703 1.381788 1.853659 1.646930 2.068233   6.485565   100

microbenchmark::microbenchmark(styler::style_text(code))
#> Unit: milliseconds
#>                      expr      min       lq     mean   median       uq       max neval
#>  styler::style_text(code) 86.46711 123.8118 147.6735 151.8006 167.6296  225.5865   100

Created on 2018-09-12 by the reprex package (v0.2.0).

Well no I think it's a bad idea because we said transformations should not be hard-coded, but go into the transformers.

@wlandau

This comment has been minimized.

wlandau commented Oct 30, 2018

Glad @lorenzwalthert pointed me to this thread again. Allowing the parser to reverse arrows seems to create an edge case.

parse(text = "function(){} -> f")[[1]]
#> function() f <- {
#> }
@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Oct 30, 2018

Good to know.

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