-
Notifications
You must be signed in to change notification settings - Fork 71
Introduce line-breaks in multiline arguments #1244
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
base: main
Are you sure you want to change the base?
Conversation
…b/styler into f209-newline-multiline-args
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
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.
This comment was marked as outdated.
This comment was marked as outdated.
|
@lorenzwalthert Any thoughts? I am blocked here without any further feedback from the team. |
|
Maybe we should talk about the abstract rule first. What is it exactly that we try to achieve? Also important: in the example you gave, all line breaks should simply be removed if we had #247. |
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
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.
This comment was marked as outdated.
|
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: 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 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. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@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. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@IndrajeetPatil ok, let's leave it open. I might implement the above outlined solution at some point, God knows when 🤔 |
This comment was marked as outdated.
This comment was marked as outdated.
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 99b9681 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces automatic line-break formatting for multiline arguments in function calls, improving code readability when some arguments span multiple lines. The main implementation adds a new transformer function set_line_breaks_for_multiline_args that ensures consistent formatting by placing line breaks after commas when at least one argument is multiline.
Key changes:
- New transformer
set_line_breaks_for_multiline_argsthat detects multiline arguments and adds appropriate line breaks - Integration into the tidyverse style guide as a strict-mode transformer
- Test cases updated to reflect the new formatting behavior, including removal of FIXME comments that are now addressed
- Incidental typo fix changing
prin(x)toprint(x)across test files
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| R/rules-line-breaks.R | Implements new set_line_breaks_for_multiline_args transformer function that adds line breaks for function calls with multiline arguments |
| R/style-guides.R | Integrates the new transformer into tidyverse_style as a strict-mode line break manipulator |
| tests/testthat/test-transformers-drop.R | Adds the new transformer name to the expected list of line break transformers |
| tests/testthat/line_breaks_fun_call/named_arguments-*.R | Test cases demonstrating the new formatting behavior for function calls with mixed single-line and multiline arguments |
| tests/testthat/line_breaks_fun_call/unindent-out.R | Updated expected output showing proper line breaks after opening parentheses and commas when multiline arguments are present |
| tests/testthat/line_breaks_fun_call/token_dependent_complex_strict-out.R | Updated expected output for complex nested function calls with multiline arguments |
| tests/testthat/roxygen-examples-complete/*-out.R | Updated expected output for roxygen examples showing the new line-break formatting |
| tests/testthat/line_breaks_and_other/pipe-line-breaks-.R | Removes FIXME comments now addressed by the new formatting; shows proper line breaks for piped expressions as function arguments |
| tests/testthat/indention_multiple/overall-*.R | Updated to reflect new formatting and incidental typo fix (prin → print) |
| tests/testthat/scope-/scope_none-.R | Incidental typo fix changing prin(x) to print(x) |
| tests/testthat/cache-with-r-cache/mlflow-1-in.R | Updated expected formatting for multiline argument in paste function |
| styler.Rproj | ProjectId regenerated (auto-generated RStudio project setting) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pd | ||
| } | ||
|
|
||
|
|
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new function set_line_breaks_for_multiline_args lacks documentation. Consider adding a roxygen2 comment block explaining:
- The purpose of the function
- The logic/rules it implements
- Example input/output (similar to
set_line_break_before_curly_openingat line 1) @keywords internaltag to match the pattern of other internal functions in this file
| #' Set line breaks for multiline function arguments | |
| #' | |
| #' Rule: | |
| #' If any argument in a function call is multiline, ensure a line break after the opening parenthesis | |
| #' and after each comma separating arguments. This improves readability for calls with complex or | |
| #' multiline arguments. | |
| #' | |
| #' Example: | |
| #' # Before: | |
| #' fun(arg1, arg2, | |
| #' long_expr + | |
| #' more_stuff, arg4) | |
| #' | |
| #' # After: | |
| #' fun( | |
| #' arg1, | |
| #' arg2, | |
| #' long_expr + | |
| #' more_stuff, | |
| #' arg4 | |
| #' ) | |
| #' | |
| #' @keywords internal |

cf. tidyverse/style#209
Created on 2024-12-20 with reprex v2.1.1