Find changed files and apply a different `.lintr` config

[dragosmg]

@dragosmg Do you have an idea if we could somehow (e.g. by setting options(lintr.linter_file) somewhere and doing some git magic to find changed files only) apply a different .lintr config to changed files in new PRs?

It would be awesome if the final set of linters could be automatically applied to all modified files in a PR without having to have the rest of the code lint red.

Originally posted by @AshesITR in #584 (comment)

[dragosmg]

@AshesITR let me see if I got this right: we want to be able to lint changed files with different (more restrictive) configurations especially while de-linting the rest of the package.

I'm having a couple of ideas:

• maybe have a separate workflow that doesn't lint the entire package (with lintr::lint_package()) but rather just the changed files.
• this workflow should use a different config file, e.g. .lintr_new, which is, at present, more restrictive than .lintr
• once the de-linting is done .lintr_new and .lintr should be identical and the separated workflow for changed files becomes superfluous.
• steps:
  • set options(lintr.linter_file = ".lintr_new")
  • extract the changed files (probably using the GitHub API rather than git) - I assume we're only interested in linting the .R files
  • lint the changed .R files

If I can get this to work the only downside I see is that the changed files will end up going through linting twice (once as part of the package lint - with less restrictive standards and once as individual files with the more restrictive config).

@russHyde @AshesITR let me know what you think.

[AshesITR]

That sounds about right.
We should also lint Rmds etc. from the changeset.
Also note there may be exclusions in .lintr_new so that e.g. linter test files can be merged without de-linting.

[dragosmg]

@russHyde @AshesITR I think I might have a working solution. Workflow in action can be seen in one of my toy packages - https://github.com/dragosmg/dmgr/actions/runs/394890057/workflow.

1. extract the changed files from the PR
2. figure out which files are .R or .Rmd
3. use a different configuration - via options(lintr.linter_file = ".lintr_new")
4. apply lintr::lint_package() excluding all files that are not identified at point 2. This is a slight workaround since lintr::lint() isn't vectorised. This double exclusion might have unforeseen side-effects.

Questions:

• what would be a good name for this new lintr config file?
• do we want to lint more than .R or .Rmd files?
• would it make sense to exclude the changed files from the overall lint workflow (this second one will be called something like lint changed files) - the only slight issue is that it complicates the lint workflow a bit, but it can be done.

[AshesITR]

Thanks for working on this - looks very promising.
To me .lintr_new sounds allright as a file name for lintr config applied to new code.

The file types linted should be consistent with the default pattern of lint_package() (i.e. including other knitr formats)
As such, wouldn't it be easiest to not give a pattern argument to lint_package() and only exclude all unchanged files?

With regards to exclusion of the changed files from the regular lint workflow - I don't think the additional complexity is worth the (probably miniscule) gain in performance and the risk of .lintr_new somehow becoming accidentally less restrictive than .lintr without us immediately noticing.

[dragosmg]

Excluding all unchanged files sounds straightforward. However, I am getting a complaint from lintr::lint_package() if I try to pass a list of exclusions comprising of all unchanged files.

Error in normalize_exclusions(c(exclusions, settings$exclusions), root = path,  : 
  object 'pattern' not found

Somewhat of a reprex

files <- gh::gh("GET https://api.github.com/repos/jimhester/lintr/pulls/652/files")
changed_files <- purrr::map_chr(files, "filename")

# works with a pattern
# files_to_lint <- changed_files[grepl(".R$|.Rmd$", changed_files)]
# all_files <- fs::dir_ls(recurse = TRUE, glob = ".Rmd|.R")

# fails without a pattern - with the error mentioned above
all_files <- fs::dir_ls(recurse = TRUE)
exclusions_list <- as.list(setdiff(all_files, changed_files))
lintr::lint_package(exclusions = exclusions_list)

I'll try to get to the bottom of this. I'm not super familiar with the codebase. In the meantime, if you have any ideas, they would be greatly appreciated.

[dragosmg]

Update: replacing fs::dir_ls(...) with list.files(...) seems to solve the problem.

[dragosmg]

@AshesITR I think the only outstanding thing is the content of .lintr_new.

Does this still work in terms of content?

linters: list(
    assignment_linter,
    line_length_linter = line_length_linter(160), # TODO: reduce to 120
    no_tab_linter,
    pipe_continuation_linter,
    seq_linter,
    single_quotes_linter,
    trailing_whitespace_linter
  )
exclusions: list(
  "tests",  # temporarily ignore all of tests
  "tests/testthat/dummy_packages",
  "tests/testthat/knitr_formats",
  "tests/testthat/knitr_malformed",
  "inst",
  "vignettes"
 )

[russHyde]

We can simplify that a bit. For newly written/modified code, the lintr config could be:

linters: with_defaults(
    line_length_linter = line_length_linter(120),
    T_and_F_symbol_linter = T_and_F_symbol_linter
  )
exclusions: list(
  "tests/testthat/dummy_packages",
  "tests/testthat/knitr_formats",
  "tests/testthat/knitr_malformed",
  "inst",
  "vignettes"
 )

That is, we should include any newly modified test-code in the stricter linting workflow, and we want to apply the default-linters (with a couple of modifications) to any new code

[AshesITR]

I'll try to get T_and_F_symbol_linter into the defaults today.

[dragosmg]

Cool. I think the PR is pretty much good to go as far as I am concerned. Let me know if we need to make any changes to .lintr_new related to T_and_F_symbol_linter.

[AshesITR]

It's merged now so the line can be deleted.

[AshesITR]

@russHyde the old cyclocomp setting used to be 15. Do we want to revert to the default of 25? This would also remove work for the delinting project since most current lints are in the 15 - 25 range.

[dragosmg]

Done. I've removed the line from .lintr_new. Hopefully all will work as expected. I've tested it in my toy package, but that's a far less complex package than {lintr}.