Adapt to single indent semantics in style guide

[krlmlr]

https://style.tidyverse.org/functions.html#multi-line-function-definitions

It's merely a start, doesn't capture all edge cases yet.

Doesn't conflict with #1137 except for the currently last commit that would be easy to do later on.

CC @MichaelChirico.

[github-actions]

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

• ✔️cache_applying: 166ms -> 168ms [-2.56%, +5.29%]
• ✔️cache_recording: 521ms -> 520ms [-0.83%, +0.42%]
• ✔️without_cache: 998ms -> 996ms [-1.16%, +0.57%]

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

[IndrajeetPatil]

The changes look good from my end. If you want, you can also add tests for anonymous function and nested function cases for a good measure.

library(styler)

"purrr::map(vec, function(
    x
) NULL)" -> code1

style_text(code1)
#> purrr::map(vec, function(
#>   x
#> ) {
#>   NULL
#> })

"function(
    x,
    f = function(
    x
    ) (x)^2
) NULL" -> code2

style_text(code2)
#> function(
#>   x,
#>   f = function(
#>     x
#>   ) {
#>     (x)^2
#>   }
#> ) {
#>   NULL
#> }

^{Created on 2024-10-23 with reprex v2.1.1}

---

Outside the scope of the current PR, but this looks a bit off:

function(
  x,
  y) {
  NULL
}

compared to

function(
  x,
  y
) {
  NULL
}

I don't know if the style guide has changed its opinion on supporting the latter in the latest round of updates, though.

[MichaelChirico]

Oh, just realized I'm cc'd. test output LGTM. I might like some cases with comments, especially comments in weird places like

f = function(
a
# comment
= 1
) { a }

[lorenzwalthert]

Do I understand correctly from the related style guide PRs that in no situation, double indent is allowed?

[lorenzwalthert]

This looks odd. Need to rename is_double_indention?

[lorenzwalthert]

@krlmlr can you fix this? You have

    is_double_indention <- is_single_indent_function_declaration(pd)

I think we need to rename the variable name and keep the function name. Then I'll approve the PR and we can merge.

[krlmlr]

Yeah, we moved away from double indent to keep things similar with Rust, IIRC. Upstream: tidyverse/style#223 .

[krlmlr]

This is now strictly in a "works for me" state. If anyone wants to take over, I'm more than happy to do a final review. Otherwise, I'll fix problems as I notice them.

[lorenzwalthert]

If we don't have double indention anymore anywhere, can't we just (broadly speaking) revert #1083? I.e. the indent rules on braces that result from function(\n...) should already result the correct indention?

[krlmlr]

I'm not sure. Double indent becomes single indent, same concept, but with two instead of four spaces.

[MichaelChirico]

That's not quite true. the key is where the formals-closing paren and body-opening brace go has changed.

https://style.tidyverse.org/functions.html#multi-line-function-definitions

Double indent:

foo = function(
    arg1 = def1,
    arg2 = def2) {
  body
}

New single-indent:

foo = function(
  arg1 = def1,
  arg2 = def2
) {
  body
}

I don't think there was any supported way to do function(\n prior to that -- we had a lot of code like:

foo =
  function(arg1 = def1,
           arg2 = def2) {
    body
  }

i.e. hanging indent, but buy some space by bumping function itself to the next line (and indent)

[lorenzwalthert]

Yeah, I just meant that as far as indention goes (line breaks for single indent are an orthogonal topic I think), single indenting could be achieved by removing rules for double indent and rely on the intention rules for braces (they were deactivated for function declaration on purpose IIRC) while still keep the code path for hanging indent. That way, there would not be any function declaration specific indent rules, which means less code, speed up but also no explicit rule (that could be modified for custom style guide) for function declarations. So you are right, strictly reverting #1083 would not be what we wanted.

[lorenzwalthert]

I did some digging and it's actualy easier to just do what you proposed here. Using the brace as a trigger for idention will solve the single indent case, but not the hanging indent case:

function(x, 
         y)
  NULL

So lets fix the last remaining comment and then merge .

[jrosell]

In fact, it would be awesome to be able to set single indention or doble indention option in function arguments (and document it). Such as:

foo = function(
  arg1 = def1,
  arg2 = def2 
) {
  body
}

Or:

foo = function(
    arg1 = def1,
    arg2 = def2 
) {
  body
}

[lorenzwalthert]

double indent is apparently not supported in stye style guide anymore, so we'll drop support for ithere.

[krlmlr]

I'm using this PR on GitHub Actions, so far, it works for me. I don't have the capacity to wrap this up in a nice way until next year.

[lorenzwalthert]

Ok, I can finish that up.

[github-actions]

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

• ✔️cache_applying: 172ms -> 171ms [-3.11%, +2.53%]
• ✔️cache_recording: 535ms -> 535ms [-1.22%, +0.95%]
• ✔️without_cache: 1.01s -> 1.01s [-1.15%, +0.51%]

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