Who lints the linters?

[russHyde]

Although there is a .lintr file in the lintr root, linting is not done as part of the current travis-CI workflow (or the soon to be introduced GHA CI workflow).

If people are keen to integrate lintr into their dev workflow, they're probably going to look at lintr as a good example of how to set this up. For those who read docs, the current docs explain how to set up lintr for running on Travis; they might need expanding to explain how to use lintr on GHA / other providers. For those who don't read docs, it would be good if {lintr} had exemplary lint-running code that they can copy/paste into their own projects (or perhaps a use_lintr hook could be constructed for {usethis} that makes setting up linting trivial).

Nonetheless, the current linting setup fails:

With master: f878ec9
if we run Rscript -e "devtools::load_all(); lint_package()" in the lintr root, we get the following:

Loading lintr
Error: Malformed file!
Execution halted

If we run Rscript -e "devtools::load_all(); lint_dir('R')" we get a long list of lints. (and also for lint_dir("tests"))

Some of these lints

• are due to bugs in lintr (some fixed in recent but unmerged PRs: paren_brace_linter false positive within regex strings: "(match_group){quantifier}" #441, Equals na comment #546
• are due to the "exclusions" not being updated when example/test code is added that is designed to fail the linters (@AshesITR 's recent directory-exclusion PR should ease fixing this kind of stuff)
• would have been fixed if linting had been a consistent part of the lintr PR workflow.

Should we fixup the lintr PR process so that linting is always run?
And if we do, how do we change the current code base so that linting the current code comes back green?

What we shouldn't do is a monolithic PR using {styler} or something that fixes hundreds of function(arg=123) to function(arg = 123); when these wholesale-change commits introduce bugs they are hell to fix.

I'd suggest we rewrite the .lintr:

• exclude all test & doc directories for now
• use the smallest collection of the default linters that come back green on <lintr>/R
• apply these linters to any newly submitted PR
• then do individual PRs to gradually add further default-linters back in

An alternative would be to write up a .lintr that defines the linters that we definitely do want to apply to lintr and write up a CI workflow script that applies these linters to just those files that were modified in a submitted PR; and if a lint crops up on a line that was touched by the PR author then linting should return red from CI.

[AshesITR]

Here are some stats on babe9f7 (current HEAD of #539) for lintr::lint_package():

By linter

linter	n
infix_spaces_linter	204
spaces_inside_linter	56
paren_brace_linter	26
line_length_linter	22
object_name_linter	18
commented_code_linter	15
open_curly_linter	15
assignment_linter	13
trailing_blank_lines_linter	13
cyclocomp_linter	11
object_usage_linter	10
closed_curly_linter	8
equals_na_linter	6
object_length_linter	5
trailing_whitespace_linter	5
spaces_left_parentheses_linter	4
function_left_parentheses_linter	3
single_quotes_linter	3
commas_linter	2
no_tab_linter	1
pipe_continuation_linter	1
seq_linter	1

R/ by linter

linter	n
infix_spaces_linter	22
commented_code_linter	11
line_length_linter	11
cyclocomp_linter	10
open_curly_linter	10
object_name_linter	9
spaces_inside_linter	7
closed_curly_linter	5
object_length_linter	3
paren_brace_linter	3
equals_na_linter	2
function_left_parentheses_linter	2
object_usage_linter	2
spaces_left_parentheses_linter	2
trailing_blank_lines_linter	2
commas_linter	1

By filename

filename	n
tests\testthat\test-object_name_linter.R	56
tests\testthat\test-semicolon_terminator_linter.R	48
tests\testthat\default_linter_testcode.R	28
tests\testthat\test-absolute_path_linter.R	25
tests\testthat\test-extraction_operator_linter.R	22
tests\testthat\test-implicit_integer_linter.R	21
R\get_source_expressions.R	19
tests\testthat\test-unneeded_concatenation_linter.R	19
tests\testthat\test-undesirable_function_linter.R	17
tests\testthat\test-T_and_F_symbol_linter.R	15
tests\testthat\test-todo_comment_linter.R	13
R\object_name_linters.R	12
tests\testthat\test-undesirable_operator_linter.R	11
R\object_usage_linter.R	9
R\utils.R	9
tests\testthat\test-pipe_continuation_linter.R	9
R\expect_lint.R	6
R\unneeded_concatenation_linter.R	6
R\path_linters.R	4
tests\testthat\knitr_formats\test.Rhtml	4
tests\testthat\knitr_formats\test.Rmd	4
tests\testthat\knitr_formats\test.Rnw	4
tests\testthat\knitr_formats\test.Rrst	4
tests\testthat\knitr_formats\test.Rtex	4
tests\testthat\knitr_formats\test.Rtxt	4
tests\testthat\test-closed_curly_linter.R	4
tests\testthat\test-get_source_expressions.R	4
R\commas_linter.R	3
R\lint.R	3
R\spaces_inside_linter.R	3
R\T_and_F_symbol_linter.R	3
R\undesirable_operator_linter.R	3
R\zzz.R	3
tests\testthat\test-equals_na_linter.R	3
tests\testthat\test-function_left_parentheses_linter.R	3
tests\testthat\test-lint_package.R	3
tests\testthat\test-spaces_left_parentheses_linter.R	3
R\closed_curly_linter.R	2
R\equals_na_lintr.R	2
R\namespace.R	2
R\open_curly_linter.R	2
R\semicolon_terminator_linter.R	2
R\undesirable_function_linter.R	2
tests\testthat\test-exclusions.R	2
tests\testthat\test-expect_lint.R	2
tests\testthat\test-nonportable_path_linter.R	2
tests\testthat\test-paren_brace_linter.R	2
R\actions.R	1
R\cyclocomp_linter.R	1
R\extraction_operator_linter.R	1
R\function_left_parentheses.R	1
R\namespace_linter.R	1
R\paren_brace_linter.R	1
R\sprintf_linter.R	1
tests\testthat\test-defaults.R	1
tests\testthat\test-lint_file.R	1
tests\testthat\test-open_curly_linter.R	1
tests\testthat\test-seq_linter.R	1

By directory

dir	n
tests/testthat	316
R	102
tests/testthat/knitr_formats	24

[AshesITR]

From the stats I can see a few things that we could make as a first step PR:

1. (permanently) exclude tests/testthat/knitr_formats - those files are all designed to lint.
2. (temporarily) exclude tests as suggested
3. Fix as many lints of files in R/ in a one-by-one fashion as can be done without altering the parse tree (i.e. purely stylistic lints) making sure to create a new commit for each touched file

By making each file a single commit, potential bugs can be efficiently pinpointed with git bisect should the need arise, thereby minimising risk.

Since syntactic lints, such as cyclocomp_linter, object_name_linter or object_usage_linter can be a different beast with regards to the potential for new bugs, we should be more careful with fixing those.

[russHyde]

Can we flag as 'good-first-issue' all single-file lint fixes.

[MichaelChirico]

Would PRs by file or by linter be better? If the fix is going to introduce a bug, I guess it would be easy to introduce the same bug to multiple files, and easier to fix that same fix in multiple places at once.

[AshesITR]

For style issues I really don't mimd a large PR as long as there are commits to bisect.

For synatctic linters like cyclocomp I think we really should go function by function.

[russHyde]

Thanks for your analysis @AshesITR . Based on this, the following is a minimal .lintr that returns green on the current codebase:

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]

But ultimately, we'd aim to get this back to the following (the original settings, but disregarding all test-examples)

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

[AshesITR]

Do you think it would be useful to keep a long-running PR going with the final .lintr file and periodically merge master into it to check how the de-linting goes?

If so, we'd need:

1. a Kick-Off PR with the minimal .lintr, cf. @russHyde's comment
2. a restoration PR with the final .lintr file, to be kept open until all lints are gone
3. Issues for each linter with lints in R/ as per my overview, tagged good-first-issue
4. a Meta-Issue for coordination of the de-linting process for tests/

Number 3. could alternatively be organized per file.
However, @MichaelChirico's suggestion would have the advantage that the associated PRs can add their linter to the .lintr file immediately.

cc @jimhester

[AshesITR]

@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.

[russHyde]

Are there any default linters that we don't want to apply to lintr? Some linters may need commented code to explain what they do, so maybe the commented-code linter should be dropped

[AshesITR]

The commented code can trivially be "fooled" by commenting the code with a special sequence. I like to use this:

# This is a regular comment with a line of code
#> 1 + 1 == 2

On the other hand: WDYT about using more than the default set? Such as T_and_F_symbol_linter?

[AshesITR]

The following functions have cyclocomp_linter hits:

• closed_curly_linter
• commas_linter
• fix_tab_indentations (in get_source_expressions.R)
• fix_eq_assigns (in get_source_expressions.R)
• lint
• namespace_linter
• open_curly_linter
• split_path (in path_linters.R)
• sprintf_linter
• get_tokens_in_parentheses (in unneeded_concatenation_linter.R)

[dragosmg]

On @russHyde's point about {usethis} and a potential use_lintr() function. r-lib/actions has a lintr workflow example (https://github.com/r-lib/actions/blob/master/examples/lint.yaml). It can be accessed via usethis::use_github_action("lint"). Maybe this could be included in the documentation.

[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.

@AshesITR is this still needed? If yes, I suggest we create a separate issue and look into this.

[AshesITR]

Hi @dragosmg I still think this would be useful, yes.

[lorenzwalthert]

What we shouldn't do is a monolithic PR using {styler} or something that fixes hundreds of function(arg=123) to function(arg = 123); when these wholesale-change commits introduce bugs they are hell to fix.

Just FYI when scope < 'tokens', {styler} can verify that no bugs are introduced because only spaces and line breaks are modified and the parse tree remains identical. That means we have strong guarantees here. See Round trip validation in help(style_pkg, package = 'styler'). You can also use {styler} to just style spaces, then spaces and indention, then add line breaks, then tokens. Much easier to review one by one than one bulk update, I agree. Also, you can do this by file with style_file().

Edit: The guarantees currently don't hold for roxygen code examples.

[MichaelChirico]

I think this issue can be closed -- check #1186 for the last vestige of it