more considered handling of conditions in config parsing

[MichaelChirico]

Closes #2253.

The output in @Bisaloo's case now looks like this:

withr::with_dir("/tmp/testlintrconfigerror", lint_package())
# Warning message:
# Warning encountered while loading config:
#   Warning from config setting 'linters':
#     It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.

Open to fine-tuning it a bit but feels like we're in the right direction here.

[codecov-commenter]

Codecov Report

Merging #2254 (c201a2b) into main (22636d6) will decrease coverage by 0.02%.
The diff coverage is 96.00%.

❗ Current head c201a2b differs from pull request most recent head f2e39f8. Consider uploading reports for the commit f2e39f8 to get more accurate results

@@            Coverage Diff             @@
##             main    #2254      +/-   ##
==========================================
- Coverage   99.65%   99.63%   -0.02%     
==========================================
  Files         114      114              
  Lines        5215     5235      +20     
==========================================
+ Hits         5197     5216      +19     
- Misses         18       19       +1     

Files	Coverage Δ
R/settings.R	98.34% <96.00%> (-0.67%)	⬇️

[AshesITR]

Can we get the call that produced the warning to appear?

[MichaelChirico]

Can we get the call that produced the warning to appear?

Do you just want to print the string that caused the warning (in this case, it would be "linters_with_defaults(\n backport_linter(\"2.10.0\")\n )")? Or to isolate it to backport_linter(\"2.10.0\")? The latter would require a lot more code than just eval(.). My only concern the former is printing an arbitrarily formatted string can be quite long (e.g. our own linters entry is 39 lines long), output may look scary / hard to understand in that case.

[MichaelChirico]

Here's what the call stack looks like from within the relevant warning handler:

[[17]]
eval(parsed_setting)

[[18]]
eval(parsed_setting)

[[19]]
linters_with_defaults(backport_linter("2.10.0"))

[[20]]
dots <- list(...)

[[21]]
r_version <- normalize_r_version(r_version)

[[22]]
warning("It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.")

[[23]]
.signalSimpleWarning("It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.", 
    base::quote(normalize_r_version(r_version)))

[[24]]
withRestarts({
    .Internal(.signalCondition(simpleWarning(msg, call), msg, 
        call))
    .Internal(.dfltWarn(msg, call))
}, muffleWarning = function() NULL)

[[25]]
withOneRestart(expr, restarts[[1L]])

[[26]]
doWithOneRestart(return(expr), restart)

[[27]]
(function(w) {
            browser()
            warning("Warning from config setting '", setting, "':\n    ", conditionMessage(w))
            invokeRestart("muffleWarning")
          })(list(message = "It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.", 
    call = normalize_r_version(r_version)))

w$call (what would usually be shown under call.=TRUE) is normalize_r_version(r_version).

Would including w$call be an improvement in this case?

[AshesITR]

normalize_r_version() seems worth including.
How does dplyr & friends handle the dots?
Maybe we can improve with_defaults(), too.

test <- dplyr::mutate(iris, okay = Sepal.Length + Sepal.Width, warny = warning("warn"))

Provides a very nice warning:

Warning: There was 1 warning in `dplyr::mutate()`.
i in argument: `warny = warning("warn")`.
Caused by warning:
! warn

[MichaelChirico]

IINM that would require hooking into the {rlang} error+stack system (which I don't know anything about). Not sure it's worth it for this use case.

[MichaelChirico]

Current output from the test case in the issue:

withr::with_dir("/tmp/testlintrconfigerror", lint_package())
Warning message:
Warning encountered while loading config:
  Warning from config setting 'linters' at call 'normalize_r_version(r_version)':
    It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.

[MichaelChirico]

Warning: There was 1 warning in dplyr::mutate().
i in argument: warny = warning("warn").
Caused by warning:
! warn

BTW, mutate() already evaluates its arguments one-by-one (necessary to make mutate(DF, a = 1, b = a +1) possible), which makes it more natural to include a warning message naming the culprit argument.

[AshesITR]

Fine for now. "at call..." sounds awkward to me, though.

[AshesITR]

Base R does "Error in (call): (message)".
WDYT about "Warning from config setting 'linters' in 'normalize_r_version(r_version)'..."?

[MichaelChirico]

Sure, that works

[AshesITR]

LGTM except for the nit in NEWS.