Introduce line-breaks in multiline arguments

[IndrajeetPatil]

trafficstars

cf. https://github.com/tidyverse/style/issues/209

styler::style_text("
list(
  x, y,
  c(
    a
  ), c(
    b
  )
)
")
#> list(
#>   x, y,
#>   c(
#>     a
#>   ),
#>   c(
#>     b
#>   )
#> )

^{Created on 2024-12-20 with reprex v2.1.1}

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 334ebc469dee9b8a516ebcd2b0b7dfdc9a190c2b is merged into main:

• :rocket:cache_applying: 152ms -> 146ms [-7.09%, -0.06%]
• :exclamation::snail:cache_recording: 529ms -> 1.2s [+123.19%, +130.91%]
• :exclamation::snail:without_cache: 991ms -> 2.28s [+125.11%, +134.51%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 334ebc469dee9b8a516ebcd2b0b7dfdc9a190c2b is merged into main:

• :heavy_check_mark:cache_applying: 156ms -> 151ms [-7.5%, +1.4%]
• :exclamation::snail:cache_recording: 516ms -> 1.15s [+121.01%, +125.31%]
• :exclamation::snail:without_cache: 974ms -> 2.14s [+116.85%, +122.78%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[IndrajeetPatil]

Wow, that's quite the slowdown. Need to figure out what's causing it once the scope is clear.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 58acf35906415bd331fe3b4b9fed39f8a4c4c069 is merged into main:

• :heavy_check_mark:cache_applying: 147ms -> 145ms [-3.58%, +0.91%]
• :exclamation::snail:cache_recording: 511ms -> 1.11s [+115.45%, +119.88%]
• :exclamation::snail:without_cache: 979ms -> 2.2s [+124.26%, +125.96%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 27b5eca5e0b4cf1fe2a56a0abb4122155933bc60 is merged into main:

• :heavy_check_mark:cache_applying: 164ms -> 162ms [-4.68%, +1.83%]
• :exclamation::snail:cache_recording: 542ms -> 1.27s [+128.12%, +139.62%]
• :exclamation::snail:without_cache: 1000ms -> 2.42s [+137.76%, +146.26%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[IndrajeetPatil]

@lorenzwalthert Any thoughts?

I am blocked here without any further feedback from the team.

[lorenzwalthert]

Maybe we should talk about the abstract rule first. What is it exactly that we try to achieve? As for the code, I just wanted to say that you can easily figure out if there are line-breaks in a nest by looking at the respective column. To figure out if there are line breaks hidden in one of the children, consier the is_multi_line column. That will save you a lot of computations I think.

Also important: in the example you gave, all line breaks should simply be removed if we had https://github.com/r-lib/styler/issues/247.

[IndrajeetPatil]

What is it exactly that we try to achieve?

This is where I need some clarity.

I am interpreting the discussion in https://github.com/tidyverse/style/issues/209 to mean that the style guide recommends "one line per argument" rule.

Elsewhere, the style guide carves out an exception to put multiple arguments per line, but the intent of clubbing similar arguments is not something {styler} can understand:

Therefore, I opted to just always follow the stated rule and if users wish to retain multiple arguments per like, they will just have to add ignore directives.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6df06869b59e6e41b1b6e392ef59825dc1415e86 is merged into main:

• :heavy_check_mark:cache_applying: 182ms -> 180ms [-3.64%, +1.13%]
• :exclamation::snail:cache_recording: 558ms -> 1.29s [+126.56%, +136.27%]
• :exclamation::snail:without_cache: 1.05s -> 2.61s [+145.23%, +154.09%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[lorenzwalthert]

as it is evident from the test cases, enforcing one argument per line is quite a change to the current behaviour I'd discourage this, at least at this point. Also, sometimes you want more than one argument per line when named arguments are involved:

map(x, f
  y = 2
)

So I suggest to just focus on a small interesting scope first and then maybe extend it gradually. And in my view, a small improvement involves the focus of multi-line arguments and it is to enforce a line-break between two multi-line arguments using the is_multi_line argument.

c(
 c(
  'b'
 ), fun(
  f = 2
 )
)
# becomes 
c(
 c(
  'b'
 ), 
 fun(
  f = 2
 )
)

Or even slightly more rigid, each multi-line expression has to be on its own line:

c(
 a, c(
  'b'
 ), fun(
  f = 2
 )
)
# becomes 
c(
 a, 
 c(
  'b'
 ), 
 fun(
  f = 2
 )
)

Comments should probably be ignored in that logic, i.e. token after or before comma should be defined as in skip the comments. Do you want to adapt your PR to that effect? I can also tell you how I'd solve the puzzle in abstract terms first, but I don't want to take away the fun of figuring it out, so you can go ahead if you want.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 7525889d07e34de85b610544c14c4c7112f1c7fa is merged into main:

• :rocket:cache_applying: 162ms -> 159ms [-4.02%, -0.08%]
• :exclamation::snail:cache_recording: 534ms -> 1.09s [+103.17%, +105.7%]
• :exclamation::snail:without_cache: 1s -> 2.39s [+137.01%, +139.91%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if c624bfcecdf31d667f5eaf144a9a2bc6a5b7add4 is merged into main:

• :heavy_check_mark:cache_applying: 189ms -> 186ms [-6.59%, +3.31%]
• :exclamation::snail:cache_recording: 575ms -> 1.25s [+112.43%, +121.28%]
• :exclamation::snail:without_cache: 1.07s -> 2.79s [+156.09%, +163.95%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[IndrajeetPatil]

@lorenzwalthert I am not sure how to fix the performance issues here, and I no longer have time to work on this, at least not in the forseeable future.

I can either close the PR, or let you take it over from here. Up to you.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 80292c0d2593b9f88baa40dd0c2a0d4def6bfab4 is merged into main:

• :heavy_check_mark:cache_applying: 26.6ms -> 26.6ms [-2.35%, +2.44%]
• :exclamation::snail:cache_recording: 399ms -> 987ms [+145.54%, +149.46%]
• :exclamation::snail:without_cache: 930ms -> 2.35s [+151.54%, +154.42%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[lorenzwalthert]

@IndrajeetPatil ok, let's leave it open. I might implement the above outlined solution at some point, God knows when 🤔

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if f5d9bc3c80327ae6fb06d8a14dcb6deca2d5a0ca is merged into main:

• :heavy_check_mark:cache_applying: 27.6ms -> 27.9ms [-2.63%, +4.97%]
• :exclamation::snail:cache_recording: 413ms -> 1.09s [+162.26%, +166.55%]
• :exclamation::snail:without_cache: 961ms -> 2.6s [+168.41%, +172.79%]

Further explanation regarding interpretation and methodology can be found in the documentation.

[github-actions[bot]]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 99b968106d5e80ab1acece147bcf26ebaf97e498 is merged into main:

• :heavy_check_mark:cache_applying: 26.8ms -> 26.7ms [-2.43%, +2%]
• :exclamation::snail:cache_recording: 404ms -> 957ms [+135.29%, +137.88%]
• :exclamation::snail:without_cache: 949ms -> 2.28s [+138.55%, +141.47%]

Further explanation regarding interpretation and methodology can be found in the documentation.