r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.16k stars 184 forks source link

`library_call_linter()` should relax for Rmd files #2161

Closed AshesITR closed 9 months ago

AshesITR commented 9 months ago

Hmm, not super common:

https://github.com/search?q=org%3Acran%20path%3Avignettes%20%2Fopts_chunk%5B%24%5Dset%2F&type=code

I'd call this a false positive of library_call_linter() which should probably exempt antecedent opts_chunk$set() calls (though our library(lintr) probably should appear higher in the vignette). I'll skip this change for now.

_Originally posted by @MichaelChirico in https://github.com/r-lib/lintr/pull/2152#discussion_r1323530530_

Maybe an easy workaround would be to add require_first=TRUE and make the linter optionally more lax in the sense that it only lints if library() calls are interrupted by other code. This would force grouping all library calls together without forcing this group all the way to the top.

AshesITR commented 9 months ago

related to #2153

MichaelChirico commented 9 months ago

I might call the argument allow_preamble = TRUE by default. Then some preamble of setup is allowed before the first library() call, but thereafter the library() calls have to be consecutive.

Here's an example of a more general preamble -- it's not likely we'll be able to write a concise description of what a "good" preamble looks like:

https://github.com/tidyverse/dplyr/blob/73742713bc61d4ef079e72b806143b23cb7c4871/revdep/check.R#L1-L8

IndrajeetPatil commented 9 months ago

I strongly feel that this linter shouldn't be run by default on .Rmd, .qmd, .Rnw, etc. files.

These are long-form documentation formats where packages are loaded (via library()) close to where they are needed, and that's the most natural form of loading them. E.g., if I have a vignette showing how to create various plots using {ggplot2}, it feels natural to load {sf} in the chunk where I show how to create a map than loading it initially alongside other packages.

Maybe we can have an additional argument that specifies the file extensions on which this linter could be run, and omit the aforementioned extensions from the default argument?

MichaelChirico commented 9 months ago

I think we can still offer something useful for Rmd+, but first get_source_expressions() need to be chunk-aware. There are a few related issues under label:rmd that would also benefit from this. The idea would be for the lint to apply on a chunk level, i.e. any library() calls have to be at the beginning of a chunk (wherever it is in the file).

AshesITR commented 9 months ago

We use Rmd for self documenting ETL pipelines. For that usecase, the linter will be good due to enabling a fail-fast principle.

Maybe we should look into allowing wildcard exclusions based on file names?

exclusions: list("*.Rmd" = "library_call_linter"))

seems readable and concise for your usecase @IndrajeetPatil. WDYT?

MichaelChirico commented 9 months ago

1976 would also be handy