Fix token insertion / comment interaction #279

lorenzwalthert · 2017-11-18T18:55:33Z

Intended to fix #278.

wrap_if_else_multi_line_in_curly() now simply forwards to wrap_if_else_multi_line_in_curly_since_is_if_expr() if the expression contains an if-clause. This should decrease complexity for the programmer.
wrap_if_else_multi_line_in_curly_since_is_if_expr() calls wrap_subexpr_in_curly() to wrap the relevant sub-expression into curly braces for both the if-clause and the else or else-if clause.

@krlmlr can you maybe add other test cases if I missed any?

krlmlr

Thanks. The examples look good, I'll try to devise a comprehensive set of examples.

krlmlr · 2017-11-18T19:49:33Z

R/indent.R

  has_if_without_curly <-
-    pd$token[1] %in% c("IF", "WHILE") && pd$child[[5]]$token[1] != "'{'"
+    pd$token[1] %in% c("IF", "WHILE") && pd$child[[expr_after_if_while]]$token[1] != "'{'"


The "WHILE" here seems inconsistent with the function name.

krlmlr · 2017-11-18T19:55:58Z

R/rules-other.R

+}
+
+
+wrap_if_else_multi_line_in_curly_since_is_if_expr <- function(pd, indent_by = 2) {


I didn't understand the function name when skimming through it. Maybe extract wrap_if_multi_line_in_curly() and wrap_else_multi_line_in_curly()and then write

wrap_if_else_multi_line_in_curly <- function(pd, indent_by = 2) { if (is_cond_expr(pd)) { pd <- wrap_if_multi_line_in_curly(pd, indent_by) pd <- wrap_else_multi_line_in_curly(pd, indent_by) } pd }

Yes, that makes sense. It's just that I thought it was simple but it turned out to be quite complicated with all the edge cases, so I guess now that it works we can refactor a bit.

krlmlr · 2017-11-18T21:42:02Z

This generates a full set of test cases. Do we need newlines to make sure the transformer is triggered?

gen <- function(x) {
  if (length(x) == 0) ""
  else {
    c(
      paste0(x[1], gen(x[-1])),
      paste0(x[1], " # comment\n", paste(x[-1], collapse = ""))
    )
  }
}

collapse <- function(x) paste(x, collapse = "\n\n")

cat(collapse(gen(c("if", "(", "TRUE", ")", "NULL"))))
#> if(TRUE)NULL
#> 
#> if(TRUE)NULL # comment
#> 
#> 
#> if(TRUE) # comment
#> NULL
#> 
#> if(TRUE # comment
#> )NULL
#> 
#> if( # comment
#> TRUE)NULL
#> 
#> if # comment
#> (TRUE)NULL
cat(collapse(gen(c("if", "(", "TRUE", ")", "NULL", " else", " NULL"))))
#> if(TRUE)NULL else NULL
#> 
#> if(TRUE)NULL else NULL # comment
#> 
#> 
#> if(TRUE)NULL else # comment
#>  NULL
#> 
#> if(TRUE)NULL # comment
#>  else NULL
#> 
#> if(TRUE) # comment
#> NULL else NULL
#> 
#> if(TRUE # comment
#> )NULL else NULL
#> 
#> if( # comment
#> TRUE)NULL else NULL
#> 
#> if # comment
#> (TRUE)NULL else NULL
cat(collapse(gen(c("if", "(", "TRUE", ")", "NULL", " else", " if", "(", "FALSE", ")", "NULL", " else", " NULL"))))
#> if(TRUE)NULL else if(FALSE)NULL else NULL
#> 
#> if(TRUE)NULL else if(FALSE)NULL else NULL # comment
#> 
#> 
#> if(TRUE)NULL else if(FALSE)NULL else # comment
#>  NULL
#> 
#> if(TRUE)NULL else if(FALSE)NULL # comment
#>  else NULL
#> 
#> if(TRUE)NULL else if(FALSE) # comment
#> NULL else NULL
#> 
#> if(TRUE)NULL else if(FALSE # comment
#> )NULL else NULL
#> 
#> if(TRUE)NULL else if( # comment
#> FALSE)NULL else NULL
#> 
#> if(TRUE)NULL else if # comment
#> (FALSE)NULL else NULL
#> 
#> if(TRUE)NULL else # comment
#>  if(FALSE)NULL else NULL
#> 
#> if(TRUE)NULL # comment
#>  else if(FALSE)NULL else NULL
#> 
#> if(TRUE) # comment
#> NULL else if(FALSE)NULL else NULL
#> 
#> if(TRUE # comment
#> )NULL else if(FALSE)NULL else NULL
#> 
#> if( # comment
#> TRUE)NULL else if(FALSE)NULL else NULL
#> 
#> if # comment
#> (TRUE)NULL else if(FALSE)NULL else NULL

lorenzwalthert · 2017-11-18T22:39:15Z

Ok, thanks for generating the test cases. I will incorporate them in a new test and try to ensure they work.

codecov-io · 2017-11-18T23:18:39Z

Codecov Report

Merging #279 into master will decrease coverage by 1.68%.
The diff coverage is 76%.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage   92.17%   90.49%   -1.69%     
==========================================
  Files          28       28              
  Lines        1188     1262      +74     
==========================================
+ Hits         1095     1142      +47     
- Misses         93      120      +27

Impacted Files	Coverage Δ
R/serialized_tests.R	`72% <0%> (-21.51%)`	⬇️
R/indent.R	`95.38% <100%> (+0.22%)`	⬆️
R/rules-other.R	`100% <100%> (ø)`	⬆️
R/utils.R	`65.38% <100%> (-17.95%)`	⬇️
R/token-create.R	`100% <100%> (ø)`	⬆️
R/expr-is.R	`94.73% <90%> (-5.27%)`	⬇️
R/dplyr.R	`90.9% <0%> (+3.89%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 105933c...5413eb2. Read the comment docs.

lorenzwalthert · 2017-11-18T23:20:59Z

Ok, now all valid test cases seem to work. There were a few that are invalid, I commented them out. I just c/p your code above, probably should do some documentation at some point.

krlmlr

Thanks, tests look good. Did you change much to cover all edge cases?

krlmlr · 2017-11-19T20:57:59Z

R/indent.R

@@ -78,18 +78,21 @@ indent_without_paren_for_while_fun <- function(pd, indent_by) {
 #' @describeIn update_indention Is used to indent if and if-else statements.
 #' @importFrom rlang seq2
 indent_without_paren_if_else <- function(pd, indent_by) {
+  expr_after_if_while <- next_non_comment(pd, which(pd$token == "')'")[1])


So, why is the function called indent_without_paren_if_else(), and the code is looking for "IF" and "WHILE" tokens?

Yes, that's inconsistent. I think it's no longer needed since the function indent_without_paren_for_while_fun() handles the "WHILE" case.

Related: Since we now change

if (TRUE) return(call(a))

Into

if (TRUE) { return(call(a)) }

We should probably also change

while (FALSE) xyz(v)

Into

while (FALSE) { xyz(v) }

The same applies to for. Should we open an issue and postpone it?

New issue and postponing sounds great. Do we fix the inconsistency in this PR?

Oh, just saw the commit. Thanks!

lorenzwalthert · 2017-11-19T21:40:12Z

Maybe we also need to test for strict = FALSE?

krlmlr · 2017-11-19T21:44:49Z

If you expect the output to be different, this sounds like a good idea.

…for_while_fun

lorenzwalthert · 2017-11-19T21:55:52Z

Yes it should be quite different since no braces should be added.

add new trees

lorenzwalthert · 2017-11-21T08:24:18Z

Ok, tests added

krlmlr · 2017-11-21T08:33:29Z

R/utils.R

+#' @param pos The position of the token to start the search from.
+extend_if_comment <- function(pd, pos) {
+  if (pos == nrow(pd)) return(pos)
+  if (pd[pos + 1,]$token == "COMMENT") {


Probably not important here, but in general pd$token[pos + 1] requires a lot less work. Maybe we can search later for ,] and , ] to find this and other instances?

So that's for speed I assume...

reprex::reprex_info() #> Created by the reprex package v0.1.1.9000 on 2017-11-21 library("tidyverse") #> Loading tidyverse: ggplot2 #> Loading tidyverse: tibble #> Loading tidyverse: tidyr #> Loading tidyverse: readr #> Loading tidyverse: purrr #> Loading tidyverse: dplyr #> Conflicts with tidy packages ---------------------------------------------- #> filter(): dplyr, stats #> lag(): dplyr, stats tbl <- as_tibble(mtcars) microbenchmark::microbenchmark(tbl$cyl[1], tbl[1, ]$cyl) #> Unit: microseconds #> expr min lq mean median uq max neval #> tbl$cyl[1] 47.232 50.143 62.46642 60.7610 63.6915 190.264 100 #> tbl[1, ]$cyl 254.001 298.076 569.41824 318.3655 338.8025 13161.350 100

I just checked and I found , ]$ and ,]$ only twice and I changed it as part of this PR to your suggested style.

lorenzwalthert · 2017-11-21T08:40:16Z

Not sure whether you spotted that but because of the structure of the nested parse table, end-of-expression comments are located one level up in the nest, so it's impossible to include them within the braces I think.
Hence

if (TRUE)
  4 else 
  44 # comment

Becomes

if (TRUE) {
  4 
  } else {
  44 
} # comment

Instead of

if (TRUE) {
  4 
  } else {
  44 # comment
}

Here you see why:

reprex::reprex_info()
#> Created by the reprex package v0.1.1.9000 on 2017-11-21

code <- 
"if (TRUE)
    4 else 
44 # "
styler:::create_tree(code)
#>                                                  levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {pos_id})
#> 2   ¦--expr:  [0/1] {1}                                   
#> 3   ¦   ¦--IF: if [0/1] {2}                               
#> 4   ¦   ¦--'(': ( [0/0] {3}                               
#> 5   ¦   ¦--expr:  [0/0] {5}                               
#> 6   ¦   ¦   °--NUM_CONST: TRUE [0/0] {4}                  
#> 7   ¦   ¦--')': ) [0/4] {6}                               
#> 8   ¦   ¦--expr:  [1/1] {8}                               
#> 9   ¦   ¦   °--NUM_CONST: 4 [0/0] {7}                     
#> 10  ¦   ¦--ELSE: else [0/0] {9}                           
#> 11  ¦   °--expr:  [1/0] {11}                              
#> 12  ¦       °--NUM_CONST: 44 [0/0] {10}                   
#> 13  °--COMMENT: #  [0/0] {12}
styler::style_text(code)
#> if (TRUE) {
#>   4
#> } else {
#>   44
#> }  #

krlmlr · 2017-11-21T08:56:30Z

I'm fine with putting the comment after the terminal brace.

- Adapt documentation (#290). - Add roundtrip (#287). - Fix AppVeyor builds. - Fix token insertion / comment interaction (#279). - Clarify labelling strategy (#285). - Fixing and extending Rstudioaddins (#283). - Fix eq assign parsing (#276). - style_files -> vectorized style_file (#273). - Refactoring (#270). - Fix CI (#275). - Fix covr (#274). - Renaming files (#271). - Handle styling of an unsaved active file (#243). - Test R 3.1 and R 3.2 (#249). - Allow empty {} without line break (#261). - Wrap expr in expr before enclosing with curly braces (#263). - Avoid checking for hard-coded dot (#262). - Account for dependency renaming (utf8 changed to enc) (#264). - Indention of function declaration and closing braces (#260). - Only remove line break before closing with strict option (#252).

lorenzwalthert added 8 commits November 18, 2017 16:14

draft for more in-depth testing of brace adding

70d48bd

flexiblize next_non_comment

d9a1181

don't wrap expression in expr anymore when adding curly braces.

4ccd39f

use next_non_comment where appropriate, redesign wrap_expr_in_curly

e09410d

wip. can add tokens correctly, line break missing

cc0aa0d

fix newline issue.

7185677

tidy up if-part

9aa2f6f

tidy up else part.

957309e

lorenzwalthert changed the title ~~Use next non comment~~ Fix token insertion / comment interaction Nov 18, 2017

lorenzwalthert requested a review from krlmlr November 18, 2017 18:56

lorenzwalthert mentioned this pull request Nov 18, 2017

Comments in if clause without braces not recognised correctly #278

Closed

krlmlr approved these changes Nov 18, 2017

View reviewed changes

lorenzwalthert added 4 commits November 19, 2017 00:07

add systematic tests

43f8d79

adding test out, comment out invalid cases

a9e81e3

add generator functions (bare)

838ec49

fix other edge cases

a9ab9ba

lorenzwalthert added 3 commits November 19, 2017 13:44

actually call tests

7133b5b

split wrapping of if and else part into two functions.

ac993a6

add documentation for generate_test_samples

a08cc3a

krlmlr approved these changes Nov 19, 2017

View reviewed changes

drop unnecessary while check that is handled by indent_without_paren_…

4e175d3

…for_while_fun

lorenzwalthert mentioned this pull request Nov 19, 2017

Also adding curly braces to expressions other than conditional statements #286

Closed

adding non-strict test cases

b0e0bdd

add new trees

lorenzwalthert force-pushed the use-next-non-comment branch from 33fb7e2 to b0e0bdd Compare November 21, 2017 07:47

remove redundant test cases.

51261bb

krlmlr approved these changes Nov 21, 2017

View reviewed changes

improve for speed (and legibility)

5413eb2

lorenzwalthert merged commit d665ee8 into r-lib:master Nov 21, 2017

lorenzwalthert deleted the use-next-non-comment branch November 21, 2017 09:05

lorenzwalthert mentioned this pull request Dec 8, 2017

Style with styler #297

Merged

		}


		wrap_if_else_multi_line_in_curly_since_is_if_expr <- function(pd, indent_by = 2) {

Fix token insertion / comment interaction #279

Fix token insertion / comment interaction #279

Uh oh!

Conversation

lorenzwalthert commented Nov 18, 2017

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krlmlr commented Nov 18, 2017

Uh oh!

lorenzwalthert commented Nov 18, 2017

Uh oh!

codecov-io commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lorenzwalthert commented Nov 18, 2017

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert commented Nov 19, 2017

Uh oh!

krlmlr commented Nov 19, 2017

Uh oh!

lorenzwalthert commented Nov 19, 2017

Uh oh!

lorenzwalthert commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert commented Nov 21, 2017

Uh oh!

krlmlr commented Nov 21, 2017

Uh oh!

Uh oh!

lorenzwalthert Nov 18, 2017 •

edited

Loading

codecov-io commented Nov 18, 2017 •

edited

Loading

lorenzwalthert Nov 21, 2017 •

edited

Loading