Compatibility with air + housekeeping

[lorenzwalthert]

There are surprisingly few conflicts. Now, people can format this repo with air and {styler}, with the same output. {styler} is far less invasive than air, so formatting with air once means a lot of code is not changed by {styler} on subsequent styling.

• Bug in air: Air breaks code with a variable called else_* posit-dev/air#499 caused a re-naming for convenience here.

• multi-line assignment is handled differently by {styler} than air, this can be circumvented by making sure the first line break in the statement does not occur directly after <-.

• {styler} forced no line break after first argument in switch(). This was relaxed for compatibility with air.

• We used fmt: skip in switch() statements to make the pre-commit hook pass.

• air does not respect the tidyverse rules which allow to keep unnamed argument on the first line This rule seems removed. it turns:

  map(x, f, 
    arg = 1, 
    arg = 2
  )

  Into

  map(
    x, 
    f, 
    arg = 1, 
    arg = 2
  )
  

  Which I think is not optimal.

• Another case is

    other_trigger_on_same_line <- (
      pd$token[remaining_row_idx_between_trigger_and_line_break] %in%
        other_trigger_tokens
    )

  becomes (with air)

    other_trigger_on_same_line <- (pd$token[
      remaining_row_idx_between_trigger_and_line_break
    ] %in%
      other_trigger_tokens)

  Where I also think {styler} styles superior.

• It could not preserve alignment in the blow snippet

  style_guide_name <- "styler::tidyverse_style@https://github.com/r-lib"
      create_style_guide(
        # transformer functions
        initialize             = default_style_guide_attributes,
        line_break             =        line_break_manipulators,
        space                  =             space_manipulators,
        token                  =             token_manipulators,
        indention              =         indention_manipulators,
        ...
      )

• it broke a lot of long lines, which I think is desirable. This increased the line count of the source code.

• Only single lines can be ignored, no ranges: Support skip ranges posit-dev/air#456.

[github-actions]

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

• ✔️cache_applying: 25.8ms -> 25.8ms [-1.13%, +1.1%]
• ✔️cache_recording: 349ms -> 350ms [-0.31%, +0.86%]
• ❗🐌without_cache: 856ms -> 859ms [+0.08%, +0.64%]

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

[MichaelChirico]

@randy3k recently ran a similar comparison internally -- care to share if anything further was found there?

[github-actions]

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

• ✔️cache_applying: 25.4ms -> 25.3ms [-2.52%, +1.37%]
• ✔️cache_recording: 332ms -> 333ms [-2.48%, +3.08%]
• ✔️without_cache: 853ms -> 848ms [-3.44%, +2.41%]

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

[github-actions]

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

• ✔️cache_applying: 26.3ms -> 26.1ms [-2.54%, +1.48%]
• ✔️cache_recording: 352ms -> 352ms [-0.4%, +0.85%]
• ✔️without_cache: 876ms -> 870ms [-1.23%, +0.02%]

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

[DavisVaughan]

For

map(x, f, 
  arg = 1, 
  arg = 2
)

# -> 
map(
  x, 
  f, 
  arg = 1, 
  arg = 2
)

We really try to limit the forms a function call can be in. There are really only 2:

# "Flat" (all args on same line)
map(x, f, arg = 1, arg = 2)

# "Expanded" (all args broken across lines)
map(
  x, 
  f, 
  arg = 1, 
  arg = 2
)

With one exception for trailing anonymous functions, i.e. common map() syntax of

# Allowed
map(x, function(x) {
  # do the thing
})

This has generally proved to be pretty useful in keeping complexity down, especially across intermingled Air features

This also comes into play with switch(), which is "just another function call" to Air and gets no special treatment whatsoever. So there is no support for "first arg on same line" in Air, which we have mostly found to be fine

# air will expand this
switch(x,
  a = 2,
  y = 3
)

---

posit-dev/air#199 would fix part of

  other_trigger_on_same_line <- (pd$token[
    remaining_row_idx_between_trigger_and_line_break
  ] %in%
    other_trigger_tokens)

---

posit-dev/air#114 would be opt in but would probably help with

style_guide_name <- "styler::tidyverse_style@https://github.com/r-lib"
    create_style_guide(
      # transformer functions
      initialize             = default_style_guide_attributes,
      line_break             =        line_break_manipulators,
      space                  =             space_manipulators,
      token                  =             token_manipulators,
      indention              =         indention_manipulators,
      ...
    )

[lorenzwalthert]

Thanks for following up on this @DavisVaughan

• For the map case, I think {styler} still follows the implementation predating Refine advice for long function calls tidyverse/style#226. Maybe we should follow up on that at some point, although I still think {styler}'s implementation is better (but I previously clarified that I see {styler} bound to the style guide, otherwise things become messy). The distinction with named / unnamed arguments indeed added a bit of complexity 😀.
• As mentioned, the switch() special handling gets removed in this very PR.

[lorenzwalthert]

Some more interesting cases:

  mask <- c(
    1L, 2L, if (roxygen[3L] == "\n") 3L, last(which(roxygen == "}"))
  ) %>% sort()

  call(
    1, 2, 3, 4
  )

Both are expanded, despite not maxing out the character width limit. In my opinion this consumes too much vertical space and is not optimal:

mask <- c(
  1L,
  2L,
  if (roxygen[3L] == "\n") 3L,
  last(which(roxygen == "}"))
) %>%
  sort()

call(
  1,
  2,
  3,
  4
)

In particular in the second case, one could argue this matches @DavisVaughan's description # "Flat" (all args on same line), so expansion should not happen.

[lorenzwalthert]

Another difference is that ignoring happens per line in {styler}, air ignores by expression. Since air lacks ignoring ranges, this comes in handy if the range you want to ignore coincides with a multi-line expression like a function call. Also, to ignore the subsequent expression, air places the marker above the expression to ignore, {styler} on the same line, and to exclude a range in {styler}, you put the marker above the first line to ignore, while air interprets that marker position as a marker to just ignore one expression.

I was initially inclined to set the marker text options to include both {styler}'s and airs marker text, but given that they work differently, it would probably just create more confusion.

[github-actions]

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

• ✔️cache_applying: 26ms -> 26.1ms [-1.16%, +1.64%]
• ✔️cache_recording: 358ms -> 358ms [-0.64%, +0.42%]
• ✔️without_cache: 881ms -> 879ms [-0.67%, +0.21%]

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

[lorenzwalthert]

Given the open issues + deviances from how {styler} formats the code, I am not willing to accept these changes and introduce Air compatibility here. I merged #1294, which is much smaller in scope instead.

[lorenzwalthert]

@MichaelChirico I wondered if we should write up a comparison vignette regarding Air and {styler} (I guess I am quite late to the party but anyways...). My concerns are not only code output differences, but also others, such as speed, support for file types like qmd, Rmd, Rnw, configurability, third-party integrations etc.