allow config to be an .R file

[MichaelChirico]

Closes #1210

Here's a minimal change to allow getOption("lintr.linter_file") to be an R script. My thinking is to include this in the release as an "experimental" feature & decide by 3.2.0 the future of config files for {lintr}. Some notes:

• There was some concern in the issue about "what if multiple configs are found", I'm not sure how to map that to the code, because everything is governed by the lintr.linter_file option. That points us to which config to use.
• We use the file extension to decide how to parse the file
• The change in get_setting() is a bit awkward. It might be preferable to parse() the DCF keys up-front instead.

Still needs tests/NEWS, but wanted to open discussion first.

[codecov-commenter]

Codecov Report

Merging #2177 (0d8fc9d) into main (d93437f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0d8fc9d differs from pull request most recent head 5ae8a9a. Consider uploading reports for the commit 5ae8a9a to get more accurate results

@@           Coverage Diff           @@
##             main    #2177   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         113      114    +1     
  Lines        5167     5173    +6     
=======================================
+ Hits         5149     5155    +6     
  Misses         18       18           

Files	Coverage Δ
R/methods.R	100.00% <100.00%> (ø)
R/settings.R	98.07% <100.00%> (+0.07%)	⬆️
R/settings_utils.R	100.00% <100.00%> (ø)
R/use_lintr.R	100.00% <100.00%> (ø)
R/utils.R	99.12% <100.00%> (ø)
R/zzz.R	100.00% <100.00%> (ø)

[MichaelChirico]

Ah that's annoying. IIUC we can't even write sys.source(keep.parse.data = FALSE) in the source because codetools() will complain about "possibly unused argument". I can only think of an ugly workaround...

[MichaelChirico]

(alternatively, we can just ignore the R CMD check note on R3.5.0, since CRAN is not running any checks so old anymore, but that test will just continuously fail from now on if so... or we could weaken the GHA to only fail on WARNING/ERROR and ignore NOTEs?)

[AshesITR]

Since R code can to pretty wild things, I'd suggest to add schema validation of the settings.

[MichaelChirico]

Since R code can to pretty wild things, I'd suggest to add schema validation of the settings.

Technically also affects DCF too, right? Since we just eval(parse(.)) the code there. Agree it's a bit more urgent now that we accept plain R code. I would push to follow-up to nail down what level of validation should be done as a separate issue (e.g. is isTRUE(all.equal(sappy(config, typeof) == sapply(default_settings, typeof))) sufficient?)

One thing discussed in the issue that I ended up omitting is any check that users are "doing too much" in their config. I think it's fine since we write to a "clean" environment, and only copy over the "correct" keys (instead of, say, copying over every key found in the new environment).

[AshesITR]

I would actually err on the side of safety and suggest consciously using rm() or local() if temporary variables are needed.

Re validation:

linters should be a list of linters (we already have that validation somewhere).
Exclusions should also be checked "manually".
The rest seems okay to check against types of default_settings.

But most importantly, no unused variable should be defined. And yes, the validation should be executed after loading the config from either source.

[AshesITR]

Worth a try. The process could implicitly run library(lintr) before sourcing.

[MichaelChirico]

Worth a try. The process could implicitly run library(lintr) before sourcing.

right... one issue is line numbers will be off in error messages if we just paste that into the script. is messing with an .Rprofile the best workaround? but we should be running the subprocess as --vanilla?

[AshesITR]

--vanilla can mess up local configurations with nonstandard libraries etc.

Either create a temp script that does the setup and source, or, e.g. with parallel, use clusterEvalQ() before sourcing to set up the environment.

[MichaelChirico]

--vanilla can mess up local configurations with nonstandard libraries etc.

My thinking is the script should be very plain & running into issues with --vanilla is a code smell. We can record where {lintr} itself is found and use a smarter library() call in the subprocess. Other approaches seem like overkill, to the point where I'm wanting to kill the experiment already 😅

[hadley]

IMO running this file in a subprocess is overkill, and will have a substantial performance cost for relatively little gain.

[MichaelChirico]

@AshesITR WDYT about sticking with simple sys.source() in the same session for now? Any demons that come from users running too much code with side effects in their config is "their own fault" & we are a "front end" package after all.

[AshesITR]

I'm fine with that. My original point was to run our tests in a separate process to make sure they don't depend on lintr being attached.

[MichaelChirico]

I'm fine with that. My original point was to run our tests in a separate process to make sure they don't depend on lintr being attached.

OK, added a test, let me know if that's what you had in mind.

[AshesITR]

Implementation looks good.
Should be put in a more prominent position in NEWS imo.
Technically, it is a breaking change. Or we could add a Section "Experimental features"

[MichaelChirico]

I want to take a pass at reorganizing the NEWS, let's merge for now