Use {cli} for formatting condition messages

[IndrajeetPatil]

Closes #2386

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.09091% with 18 lines in your changes missing coverage. Please review.

Project coverage is 97.96%. Comparing base (fc2268f) to head (767ab96).
Report is 9 commits behind head on main.

❗ Current head 767ab96 differs from pull request most recent head 9dcbbc6

Please upload reports for the commit 9dcbbc6 to get more accurate results.

Files	Patch %	Lines
R/shared_constants.R	42.85%	4 Missing ⚠️
R/settings.R	91.17%	3 Missing ⚠️
R/with.R	81.25%	3 Missing ⚠️
R/path_utils.R	33.33%	2 Missing ⚠️
R/utils.R	66.66%	2 Missing ⚠️
R/extract.R	50.00%	1 Missing ⚠️
R/lint.R	96.29%	1 Missing ⚠️
R/object_name_linter.R	50.00%	1 Missing ⚠️
R/object_overwrite_linter.R	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2418      +/-   ##
==========================================
- Coverage   98.15%   97.96%   -0.19%     
==========================================
  Files         125      126       +1     
  Lines        5738     5743       +5     
==========================================
- Hits         5632     5626       -6     
- Misses        106      117      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MichaelChirico]

using snapshot tests for tests capturing warnings and errors

My experience is this sets us up for a ton of toil maintaining brittle tests. Using expect_warning()/expect_error() is IMO far superior

• The entire test is self-contained, no need to reference a different file to find the expected output when reading/reviewing
• The expectation can be focused -- no need to write out the full condition message in the test, just the parts that matter. This increases clarity & reduces fragility.
• We only test things that we own. Any time {cli} changes, we are at risk of our snapshot tests breaking. Whereas the condition message fragments come directly from our sources.

[MichaelChirico]

Directional greenlight that using {cli} signals is a go for me. @AshesITR, you?

PS we should also add stop()/warning()/message() as undesirable functions in our .lintr afterwards to prevent backsliding.

[AshesITR]

I agree with both of your comments.

[IndrajeetPatil]

Btw, instead of just converting the old error messages to use {cli}, I am also using some new features that {cli} affords to improve the error messages. I think this is desirable, but you can let me know if you see any shortcomings to pursuing this further.

On main branch

library(lintr)
backport_linter("oldrel-99")
#> Error: `r_version` must be a version number or one of 'devel', 'release', 'oldrel-1', 'oldrel-2', 'oldrel-3', 'oldrel-4', 'oldrel-5', 'oldrel-6', 'oldrel-7', 'oldrel-8', 'oldrel-9'

^{Created on 2023-12-14 with reprex v2.0.2}

On feature branch

library(lintr)
backport_linter("oldrel-99")
#> Error in `normalize_r_version()` at lintr/R/backport_linter.R:37:3:
#> ! `r_version` is not valid:
#> ℹ It must be a version number or one of 'devel', 'release', 'oldrel-1',
#>   'oldrel-2', 'oldrel-3', 'oldrel-4', 'oldrel-5', 'oldrel-6', 'oldrel-7',
#>   'oldrel-8', 'oldrel-9'
#> ✖ You entered 'oldrel-99' instead
#> Backtrace:
#>     ▆
#>  1. └─lintr::backport_linter("oldrel-99")
#>  2.   └─lintr:::normalize_r_version(r_version) at lintr/R/backport_linter.R:37:3
#>  3.     └─cli::cli_abort(...) at lintr/R/backport_linter.R:96:7
#>  4.       └─rlang::abort(...)

^{Created on 2023-12-14 with reprex v2.0.2}

[IndrajeetPatil]

I am going to wait for all three reviewers to approve this before I merge it.

[MichaelChirico]

Done the R/ code, it looks great!

I will get to the tests/ changes in the next pass. Getting close!