Format all test cases with air for comparison #1256

lorenzwalthert · 2025-02-19T08:07:39Z

All test code was formatted with air and then written to the file where we previously stored the expected output of styler (with the helper script inst/air/overwrite-in-test-files-with-out-files.R). Then, I git add tests/testthat/**/*-out.R and committed. This PR was created for diff review only.

lorenzwalthert · 2025-02-19T08:19:22Z

@DavisVaughan and @lionel-, I just created the formatting diff for {styler} and air on {styler}'s test cases. As discussed, this might be helpful for you to identify (edge) cases you missed.

There are 5.5k lines of code here to be styled

$ wc -l tests/testthat/**/*-in.R | tail -n1
5680

air would re-format a substantial part of it. To me, it seems larger than expected.

github-actions · 2025-02-19T08:32:14Z

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

✔️cache_applying: 26.6ms -> 26.2ms [-3.86%, +0.92%]
✔️cache_recording: 389ms -> 388ms [-0.9%, +0.5%]
✔️without_cache: 908ms -> 907ms [-0.45%, +0.38%]

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

lionel- · 2025-02-19T11:28:44Z

@lorenzwalthert Thanks for doing this!

It seems there is a problem though. For instance that first file cols-with-one-row-in.R should have this output:

c(
  "x",
  "z",
  "cgjhg",
  "thi",
  "z"
)

c(
  "x",
  "z",
  "cgjhg",
  "thi",
  "z"
)

c(
  "x",
  "y",
  "z",
  "m",
  "n",
  "o",
  "p",
  "c",
  "d"
)

Looking at inst/air/overwrite-in-test-files-with-out-files.R, it seems like a call to air is missing?

lorenzwalthert · 2025-02-19T11:40:27Z

@lionel- inst/air/overwrite-in-test-files-with-out-files.R does only the re-naming, as the name suggests. I called air before I called the re-naming script, as air formatted in place the -in.R files, whereas our test suit writes the output to -out.R. Without the renaming (and committing right away), this PR would be the diff of input vs air, but I want output air vs output {styler}. The -in.R files are the source of the test, the -out.R files are the results. There should not be any -in.R files committed here, as the source should not be changed for the test, only the output will be different from styler (target branch) vs air (this branch, base branch).

Is it more clear now?

lionel- · 2025-02-19T12:23:56Z

gotcha, but it still looks like the formatter wasn't applied to these -out.R files?

this does not include all files since air failed for some files

lorenzwalthert · 2025-02-19T13:09:06Z

ok @lionel- my bad, something went wrong. I think air failed since some files were not formatted due to parsing errors:

lorenz@lorenzs-macbook-air-2 styler % air format tests/testthat/**/*-in.R
thread 'main' panicked at crates/air_r_parser/src/parse.rs:1210:13:
internal error: entered unreachable code: Detected non trivia character!
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Maybe it would be useful to say which ones in the CLI? Even the backtrace did not help:

lorenz@lorenzs-macbook-air-2 styler % RUST_BACKTRACE=full air format tests/
thread 'main' panicked at crates/air_r_parser/src/parse.rs:1210:13:
internal error: entered unreachable code: Detected non trivia character!
stack backtrace:
   0:        0x104722c40 - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::hadba1856081fe8dc
   1:        0x10473dbd0 - core::fmt::write::h5358bd20891469bc
   2:        0x104720094 - std::io::Write::write_fmt::hbf0611cc5d72cc91
   3:        0x104722af4 - std::sys::backtrace::BacktraceLock::print::he2302a8c253c9a13
   4:        0x104723a40 - std::panicking::default_hook::{{closure}}::hec1f77a77d7e7ffc
   5:        0x104723874 - std::panicking::default_hook::hdd59ab537dd27efb
   6:        0x104724230 - std::panicking::rust_panic_with_hook::h533a16f5f89e4278
   7:        0x104723e80 - std::panicking::begin_panic_handler::{{closure}}::h168c3a4362c8e4df
   8:        0x104723104 - std::sys::backtrace::__rust_end_short_backtrace::h601e3529ca2053df
   9:        0x104723b60 - _rust_begin_unwind
  10:        0x104763bb4 - core::panicking::panic_fmt::ha0f8363f677e0181
  11:        0x1045a3ca8 - air_r_parser::parse::RParse::derive_trivia::heb64672c0e85a8d4
  12:        0x1045a241c - air_r_parser::parse::RWalk::handle_root_leave::h3dcd0cb8369bba8c
  13:        0x1045a1c50 - air_r_parser::parse::parse_text::hbcceb7a6c1a52b83
  14:        0x1045a61c4 - tracing::span::Span::in_scope::h74f1c1be31ba357c
  15:        0x1045a1948 - air_r_parser::parse::parse_r_with_cache::h125b290ed7d8362a
  16:        0x1045a1828 - air_r_parser::parse::parse::h265d33dc5bcfac18
  17:        0x1042f5dc0 - <alloc::vec::into_iter::IntoIter<T,A> as core::iter::traits::iterator::Iterator>::fold::he497dfefa49e59ba
  18:        0x1042f9d50 - air::commands::format::format::h61a2d129dc735541
  19:        0x1042e2160 - air::run::h33158142838b6997
  20:        0x1042ce52c - air::main::h6cc1b113703e889b
  21:        0x1042cafc4 - std::sys::backtrace::__rust_begin_short_backtrace::h3ed76d43cb360c22
  22:        0x1042cafac - std::rt::lang_start::{{closure}}::he6a1bda69d5966d5
  23:        0x104719ce0 - std::rt::lang_start_internal::hacda2dedffd2edb4
  24:        0x1042ce7e4 - _main

So for all files that failed, we compared test input and styler output instead of air vs styler output before I force pushed. Since it seemed to be parallelised, I just ran air many times until not much was changing anymore with air, so I think now most files should be covered, but not sure...

github-actions · 2025-02-19T13:16:46Z

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

✔️cache_applying: 26.5ms -> 26.4ms [-2.51%, +1.68%]
✔️cache_recording: 401ms -> 401ms [-0.79%, +0.73%]
✔️without_cache: 938ms -> 936ms [-0.76%, +0.4%]

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

lionel-

Quite happy with the results overall!

lionel- · 2025-02-19T13:24:37Z

tests/testthat/alignment/fun-decs-out.R

-         tt  = NULL,
-         ayz = NULL) {}
-
+function(x = NULL, tt = NULL, ayz = NULL) {


The empty braces will be fixes as part of posit-dev/air#186

lionel- · 2025-02-19T13:25:19Z

tests/testthat/curly-curly/mixed-out.R

-call({{
-  #
-}})
+call({


This difference is expected.

lionel- · 2025-02-19T13:28:18Z

tests/testthat/escaping/basic-escape-out.R

 #' ano("\\.", further = X)
 NULL

-"single quotes with
-embedded and \n not embedded line breaks"
+'single quotes with


I opened posit-dev/air#231 for this.

lionel- · 2025-02-19T13:29:30Z

tests/testthat/indention_curly_brackets/custom-out.R

-    print(value)
-}
-
+if (value > 0) print(value)


Although we recommend side effects to be on their own line, we currently don't force it.

I wonder if we should force multi-line if the if is not assigned or part of a larger call though, since that indicates a side effect.

lionel- · 2025-02-19T13:33:00Z

tests/testthat/indention_multiple/if_else_curly-out.R

-    3
-  else
-    4
+  if (TRUE) 3 else 4
 }

 {
  if (TRUE) {


I think this will be fixed soon by @DavisVaughan's work on braces.

lionel- · 2025-02-19T13:45:28Z

tests/testthat/line_breaks_fun_call/switch_ifelse_etc_no_line_break-out.R

-  a = 2,
-  y = 3
-)
+switch(x, a = 2, y = 3)


The persistent line is only on the first argument so that's expected.

lionel- · 2025-02-19T13:45:40Z

tests/testthat/line_breaks_fun_call/token_dependent_comments-out.R

-call(call( # comment
-  3, 4
-))
+call(


Fixed already.

lionel- · 2025-02-19T13:47:08Z

tests/testthat/parse_comments/within_spacing_with_force-out.R

+#######           A comment
+#'roxygen


We don't do any normalisation of comments currently. Something to think about though.

lionel- · 2025-02-19T13:47:46Z

tests/testthat/relocate_eq_assign/eq_assign_multiple_tokens_eq_only-out.R

@@ -1,9 +1,9 @@
-a <- b <- c <- d <- e <- f <- g <- 4
+a = b = c = d = e = f = g = 4


yep this is allowed by Air. We'll have a linter to enforce a style in a repo though. Or perhaps this should be one of the few options.

Discussed in posit-dev/air#231 (comment)

lionel- · 2025-02-19T13:51:18Z

tests/testthat/roxygen-examples-complete/08-roxygen-dontrun-out.R

                       style = tidyverse_style,
                       transformers = style(...),
                       include_roxygen_examples = TRUE) {
-  changed <- withr::with_dir(
-    dirname(path),
+  changed<- withr::with_dir(


Parse error?

MichaelChirico · 2025-02-19T15:47:38Z

This is a cool thread to watch! Please notify me when it reaches a stable state (i.e. neither {styler} nor {air} will make further changes to converge to one another), it will be quite helpful.

I'll also link some seemingly related threads / issues:

r-lib/lintr#2703
r-lib/lintr#1566
tidyverse/style#160

DavisVaughan · 2025-02-19T22:35:44Z

This will fix the panic posit-dev/air#240, also air format . --log-level=trace will print extra information about the process, and with posit-dev/air#239 you'd have actually been able to see the name of the file with the problem.

After fixing the panic, running air format . on styler as a whole can now parse everything except

ERROR Failed to format tests/testthat/indention_square_brackets/square_brackets_double_line_break-in.R: Failed to parse due to syntax errors.
ERROR Failed to format tests/testthat/indention_square_brackets/square_brackets_double_line_break-out.R: Failed to parse due to syntax errors.

ERROR Failed to format tests/testmanual/addins/r-invalid.R: Failed to parse due to syntax errors.

The r-invalid.R error seems right because it does contain invalid R code

The square_brackets_double_line_break-in.R one is a bug in our tree-sitter-r implementation upstream, as this fails to parse but is technically valid R code (r-lib/tree-sitter-r#78)

a[[
  2
]
]

But overall that's pretty solid for the parser

lorenzwalthert · 2025-02-20T07:52:01Z

Sounds good. I am glad running air on our test suit helps polishing your release 😊

lorenzwalthert marked this pull request as draft February 19, 2025 08:07

lorenzwalthert force-pushed the dev-format-test-in-with-air branch from 21b9133 to c207765 Compare February 19, 2025 08:13

This comment was marked as outdated.

Sign in to view

lorenzwalthert changed the title ~~Format all test in files with air for comparison~~ Format all test cases with air for comparison Feb 19, 2025

apply air to in files, then rename them to out and commit

9092498

this does not include all files since air failed for some files

lorenzwalthert force-pushed the dev-format-test-in-with-air branch from c207765 to 9092498 Compare February 19, 2025 13:07

lionel- reviewed Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format all test cases with air for comparison #1256

Format all test cases with air for comparison #1256

lorenzwalthert commented Feb 19, 2025 •

edited

Loading

lorenzwalthert commented Feb 19, 2025

This comment was marked as outdated.

github-actions bot commented Feb 19, 2025

lionel- commented Feb 19, 2025

lorenzwalthert commented Feb 19, 2025 •

edited

Loading

lionel- commented Feb 19, 2025

lorenzwalthert commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025

lionel- left a comment

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

lionel- Feb 19, 2025

MichaelChirico commented Feb 19, 2025

DavisVaughan commented Feb 19, 2025 •

edited

Loading

lorenzwalthert commented Feb 20, 2025

		@@ -1,9 +1,9 @@
		a <- b <- c <- d <- e <- f <- g <- 4
		a = b = c = d = e = f = g = 4

		####### A comment
		#'roxygen

Format all test cases with air for comparison #1256

Are you sure you want to change the base?

Format all test cases with air for comparison #1256

Conversation

lorenzwalthert commented Feb 19, 2025 • edited Loading

lorenzwalthert commented Feb 19, 2025

This comment was marked as outdated.

github-actions bot commented Feb 19, 2025

lionel- commented Feb 19, 2025

lorenzwalthert commented Feb 19, 2025 • edited Loading

lionel- commented Feb 19, 2025

lorenzwalthert commented Feb 19, 2025 • edited Loading

github-actions bot commented Feb 19, 2025

lionel- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelChirico commented Feb 19, 2025

DavisVaughan commented Feb 19, 2025 • edited Loading

lorenzwalthert commented Feb 20, 2025

lorenzwalthert commented Feb 19, 2025 •

edited

Loading

lorenzwalthert commented Feb 19, 2025 •

edited

Loading

lorenzwalthert commented Feb 19, 2025 •

edited

Loading

DavisVaughan commented Feb 19, 2025 •

edited

Loading