Style test_that() calls to always contain code in braced expressions?

[IndrajeetPatil]

Preamble

In recent updates to {testthat}, it generates a warning if code is not in a braced expression:

library(testthat)
local_edition(3L)
test_that("testthat works", expect_equal(1, 1))
#> Warning: The `code` argument to `test_that()` must be a braced expression to
#> get accurate file-line information for failures.
#> Test passed

^{Created on 2023-11-07 with reprex v2.0.2}

But this is not something currently enforced by {styler}, and I am wondering if this is something it ought to support?

Actual output

styler::style_text('test_that("testthat works", expect_equal(1, 1))')
#> test_that("testthat works", expect_equal(1, 1))

^{Created on 2023-11-07 with reprex v2.0.2}

Expected output

styler::style_text('test_that("testthat works", expect_equal(1, 1))')
#> test_that("testthat works", {
#>   expect_equal(1, 1)
#> })

[lorenzwalthert]

Thanks. We can think about that. How does {lintr} handle it?

[MichaelChirico]

IIRC no lint currently, but it should lint.

[IndrajeetPatil]

There isn't currently a linter for this in {lintr}.

But, TBH, I don't think {lintr} needs to handle this because {testthat} itself is complaining that this is a code smell and so it'll be redundant for {lintr} to do the same. WDYT, @MichaelChirico and @AshesITR?

EDIT: Michael and I posted almost at the same time 🙈

[MichaelChirico]

testthat's diagnosis happens at run time, having these things reported at compile time by static analysis is also helpful:

• as an early reminder while you're coding
• as a code repo maintainer overseeing many others' code

[IndrajeetPatil]

That's a good point! We should also track this in {lintr} then.

[AshesITR]

I agree we could add a linter for this. See also sprintf_linter() for a similar example, where we anticipate warnings.

[lorenzwalthert]

This would require wrap_if_else_while_for_fun_multi_line_in_curly to be adapted. And since this transformer is dropped if no relevant token is found in the text to process, and the removal is based on the token and not text, this would either require to run the transformer on every text (which is expensive) or adopt the removal logic to allow for text filtering instead of only token.