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

Introduce line-breaks in multiline arguments #1244

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Dec 6, 2024

cf. tidyverse/style#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

This comment was marked as outdated.

This comment was marked as outdated.

@IndrajeetPatil
Copy link
Collaborator Author

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

This comment was marked as outdated.

This comment was marked as outdated.

@IndrajeetPatil
Copy link
Collaborator Author

@lorenzwalthert Any thoughts?

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

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 12, 2024

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 #247.

@IndrajeetPatil
Copy link
Collaborator Author

IndrajeetPatil commented Dec 18, 2024

What is it exactly that we try to achieve?

This is where I need some clarity.

I am interpreting the discussion in tidyverse/style#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:

Screenshot 2024-12-18 at 08 58 13

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.

This comment was marked as outdated.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 19, 2024

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.

@IndrajeetPatil IndrajeetPatil changed the title [WIP] Introduce line-breaks in multiline arguments Introduce line-breaks in multiline arguments Dec 20, 2024
Copy link
Contributor

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

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

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

Copy link
Contributor

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

  • ✔️cache_applying: 189ms -> 186ms [-6.59%, +3.31%]
  • ❗🐌cache_recording: 575ms -> 1.25s [+112.43%, +121.28%]
  • ❗🐌without_cache: 1.07s -> 2.79s [+156.09%, +163.95%]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants