More linters by default?

[maelle]

For instance in a recent submission I found "redundant_ifelse_linter" interesting.

I personally like running all of them but this might rather be a tip for the dev guide: https://masalmon.eu/2024/08/28/lintr-3-steps/

[mpadge]

I'm not sure where I stand on this, as linters are by definition often a matter of personal preference, and pkgcheck should by definition steer clear of such things in general ... It would all come down to which linters we might notionally agree on.

[Bisaloo]

I am maybe biased because I contributed some of them but I think some non-default linters go well beyond personal preferences and have measurable objective benefits in terms of performance. See for example https://lintr.r-lib.org/reference/matrix_apply_linter.html.

The criterion for a linter to be default is to implement the tidyverse style, but it doesn't say anything about the universality.

[mpadge]

Cross-ref ropensci-review-tools/pkgcheck#251, where this is building. Sorry @maelle, we should have been having that discussion here, so I'll paste my version of your list here instead, expecting that much of this will ultimately end up in goodpractice rather than here. Disclaimer: @maelle put all the work in compiling this list in ropensci-review-tools/pkgcheck#251; I've just copy-pasted here.

---

My opinions of which might be considered "standard" lints, as checkbox items. Comments without "MP" are from @maelle as copied across from previous more comprehensive list in ropensci-review-tools/pkgcheck#251

• lintr.r-lib.org/reference/T_and_F_symbol_linter.html
• lintr.r-lib.org/reference/any_duplicated_linter.html
• lintr.r-lib.org/reference/any_is_na_linter.html
• lintr.r-lib.org/reference/boolean_arithmetic_linter.html MP: I don't think this is likely to arise often enough
• lintr.r-lib.org/reference/class_equals_linter.html
• lintr.r-lib.org/reference/commas_linter.html MP: This fails on x[, , drop = FALSE], which I think is clearer than x[,, drop = FALSE]
• lintr.r-lib.org/reference/commented_code_linter.html (I find it especially confusing when you have to review code, better keep the idea in an issue or an Rbuildignored folder) - MP: But there are valid reasons; for example I often put URLs or issue numbers in code comments
• lintr.r-lib.org/reference/comparison_negation_linter.html
• lintr.r-lib.org/reference/condition_message_linter.html
• lintr.r-lib.org/reference/consecutive_assertion_linter.html
• lintr.r-lib.org/reference/duplicate_argument_linter.html
• lintr.r-lib.org/reference/empty_assignment_linter.html MP: Also not likely to arise often enough, and possible cases where this could aid clarity
• lintr.r-lib.org/reference/equals_na_linter.html
• lintr.r-lib.org/reference/fixed_regex_linter.html
• lintr.r-lib.org/reference/for_loop_index_linter.html
• lintr.r-lib.org/reference/function_return_linter.html
• lintr.r-lib.org/reference/if_not_else_linter.html
• lintr.r-lib.org/reference/if_switch_linter.html MP: Excluded only because we'd have to set additional max_branch_lines par to some arbitrary value
• lintr.r-lib.org/reference/ifelse_censor_linter.html
• lintr.r-lib.org/reference/implicit_assignment_linter.html
• lintr.r-lib.org/reference/inner_combine_linter.html
• lintr.r-lib.org/reference/is_numeric_linter.html
• lintr.r-lib.org/reference/keyword_quote_linter.html
• lintr.r-lib.org/reference/length_levels_linter.html
• lintr.r-lib.org/reference/length_test_linter.html
• lintr.r-lib.org/reference/library_call_linter.html MP: Example: Files that are .Rbuildignored may fail this
• lintr.r-lib.org/reference/list_comparison_linter.html
• lintr.r-lib.org/reference/literal_coercion_linter.html
• lintr.r-lib.org/reference/matrix_apply_linter.html
• lintr.r-lib.org/reference/missing_argument_linter.html
• lintr.r-lib.org/reference/nested_ifelse_linter.html
• lintr.r-lib.org/reference/nested_pipe_linter.html
• lintr.r-lib.org/reference/nrow_subset_linter.html
• lintr.r-lib.org/reference/nzchar_linter.html
• lintr.r-lib.org/reference/object_overwrite_linter.html -- or is it too strict? MP: Yes it is
• lintr.r-lib.org/reference/outer_negation_linter.html
• lintr.r-lib.org/reference/package_hooks_linter.html
• lintr.r-lib.org/reference/paste_linter.html
• lintr.r-lib.org/reference/pipe_consistency_linter.html
• lintr.r-lib.org/reference/pipe_continuation_linter.html
• lintr.r-lib.org/reference/pipe_return_linter.html
• lintr.r-lib.org/reference/print_linter.html MP: pkgcheck has to use print to generate md, html, and screen output
• lintr.r-lib.org/reference/redundant_equals_linter.html
• lintr.r-lib.org/reference/redundant_ifelse_linter.html
• lintr.r-lib.org/reference/regex_subset_linter.html
• lintr.r-lib.org/reference/rep_len_linter.html
• lintr.r-lib.org/reference/repeat_linter.html
• lintr.r-lib.org/reference/sample_int_linter.html
• lintr.r-lib.org/reference/scalar_in_linter.html
• lintr.r-lib.org/reference/semicolon_linter.html
• lintr.r-lib.org/reference/seq_linter.html
• lintr.r-lib.org/reference/sort_linter.html
• lintr.r-lib.org/reference/sprintf_linter.html
• lintr.r-lib.org/reference/stopifnot_all_linter.html
• lintr.r-lib.org/reference/string_boundary_linter.html
• lintr.r-lib.org/reference/strings_as_factors_linter.html MP: Only applies to R<4.0, so I'd say can and should be safely ignored
• lintr.r-lib.org/reference/system_file_linter.html
• lintr.r-lib.org/reference/terminal_close_linter.html
• lintr.r-lib.org/reference/undesirable_function_linter.html MP: Only for specific ones but definitely not the whole default list. And we already have a couple
• lintr.r-lib.org/reference/undesirable_operator_linter.html MP: There are good reasons for all of these; ::: should always be linted, but function does not currently suggest utils::getFromNamespace() as alternative, so doesn't help anyway.
• lintr.r-lib.org/reference/unnecessary_concatenation_linter.html
• lintr.r-lib.org/reference/unnecessary_lambda_linter.html
• lintr.r-lib.org/reference/unnecessary_nesting_linter.html - too many allow_function options there, the main base ones of which are covered elsewhere anyway.
• lintr.r-lib.org/reference/unnecessary_placeholder_linter.html MP: Specific to magrittr only, so will be decreasingly relevant
• lintr.r-lib.org/reference/vector_logic_linter.html
• lintr.r-lib.org/reference/which_grepl_linter.html

Linters that might be of general interest, for tests à la testthat

• lintr.r-lib.org/reference/conjunct_test_linter.html
• lintr.r-lib.org/reference/expect_comparison_linter.html MP: I can envision cases where the alternative is clearer (e.g. for anybody who doesn't know about expect_gt and must first figure that out)
• lintr.r-lib.org/reference/expect_identical_linter.html MP: Counter-examples include comparison of fs::path objects with character strings. Equal, not identical.
• lintr.r-lib.org/reference/expect_length_linter.html
• lintr.r-lib.org/reference/expect_named_linter.html
• lintr.r-lib.org/reference/expect_not_linter.html
• lintr.r-lib.org/reference/expect_null_linter.html
• lintr.r-lib.org/reference/expect_s3_class_linter.html
• lintr.r-lib.org/reference/expect_s4_class_linter.html
• lintr.r-lib.org/reference/expect_true_false_linter.html
• lintr.r-lib.org/reference/expect_type_linter.html
• lintr.r-lib.org/reference/yoda_test_linter.html

[maelle]

@mpadge reg "MP: I don't think this is likely to arise often enough"

But even in the case when it arises, better be safe? Plus if we eventually switch to flir, speed won't be an issue.

[maelle]

@mpadge reg "MP: But there are valid reasons; for example I often put URLs or issue numbers in code comments"

This linter is against commented code, not comments in general.

[maelle]

MP: Excluded only because we'd have to set additional max_branch_lines par to some arbitrary value

Would there be a way to use some linters for warnings? Or would cyclocomp pick up complicated logic?

[maelle]

MP: Example: Files that are .Rbuildignored may fail this

Can't we filter what files are fed to the linter?

[mpadge]

https://lintr.r-lib.org/reference/boolean_arithmetic_linter.html:

@mpadge reg "MP: I don't think this is likely to arise often enough"

But even in the case when it arises, better be safe? Plus if we eventually switch to flir, speed won't be an issue.

I was more thinking about the importance of ultimately having a set of linters which some conceivable group of "most people" would be able to "most readily understand". That one just seems too obtuse or arcane to fit in there?

---

https://lintr.r-lib.org/reference/commented_code_linter.html:

@mpadge reg "MP: But there are valid reasons; for example I often put URLs or issue numbers in code comments"

This linter is against commented code, not comments in general.

It still goes off too often to be useful for me, and I have to switch that off by default.

x <- 1 # issue ropensci-review-tools/goodpractice#189
# x <- 2 # issue ropensci-review-tools/pkgcheck#218

Stuff like that is often the best way within code to acknowledge that alternatives have been considered. Plus we ultimately want this to work well in preparation for review, during which we have to expect that people will have commented code that they may hope to clarify during review.

---

https://lintr.r-lib.org/reference/if_switch_linter.html:

MP: Excluded only because we'd have to set additional max_branch_lines par to some arbitrary value

Would there be a way to use some linters for warnings? Or would cyclocomp pick up complicated logic?

This whole issue thread here will only mean that the current default set of linters used in goodpractice is expanded. They are all just suggestions anyway, and can easily be ingored, or # nolint-ed away. That one is at a finer level than cyclocomp, so won't be picked up, but my thought there was more akin to above:

the importance of ultimately having a set of linters which some conceivable group of "most people" would be able to "most readily understand"

Apart from this one and the unnecessary nestling linter which I also rejected, all others simply work with no parameters needing to be specified at all. I think this is in the spirit of that statement, whereas even introducing a single additional linter requiring a specific parameter value risks complicating that simplicity.

---

https://lintr.r-lib.org/reference/library_call_linter.html

MP: Example: Files that are .Rbuildignored may fail this

Can't we filter what files are fed to the linter?

Yeah, but even that's tricky. Plus there are conceivable cases where library() is okay, like some kind of complex multi-threaded setup spawning a bunch of processx instances which might need library() statements separate to just passing current env. I just think this'll be too likely to fail for reasons that people may be ultimately able to justify.

As always, happy to discuss further 😄

[maelle]

Ok! I still feel strongly about the ifelse/switch one, because that's code that might be complicate reviews. If we had a way to flag it before review, it'd save time for reviewers.

[mpadge]

Ok! I still feel strongly about the ifelse/switch one, because that's code that might be complicate reviews. If we had a way to flag it before review, it'd save time for reviewers.

Just for clarity, you're referring to https://lintr.r-lib.org/reference/if_switch_linter.html, right? In which case then sure, let's go ahead with that and use defaults of (max_branch_lines = 2L, max_branch_expressions = 2L). Those seem like perfectly sensible values, and we can just hard-code them and comment no further.

[mpadge]

@MichaelChirico We'd appreciate your input on this issue, which is a proposal to include the linters listed above as default linters run by goodpractice. What do you think? Current defaults are a much smaller set:

goodpractice/R/prep_lintr.R

Lines 2 to 20 in 4a22174

	linters_to_lint <- list(
	assignment_linter = lintr::assignment_linter(),
	line_length_linter = lintr::line_length_linter(80),
	package_hooks_linter = lintr::package_hooks_linter(),
	semicolon_linter = lintr::semicolon_linter(allow_compound = TRUE),
	attach_detach_linter = lintr::undesirable_function_linter(
	fun = lintr::default_undesirable_functions[c("attach", "detach")]
	),
	setwd_linter = lintr::undesirable_function_linter(
	fun = lintr::default_undesirable_functions["setwd"]
	),
	sapply_linter = lintr::undesirable_function_linter(
	fun = lintr::default_undesirable_functions["sapply"]
	),
	library_require_linter = lintr::undesirable_function_linter(
	fun = lintr::default_undesirable_functions[c("library", "require")]
	),
	seq_linter = lintr::seq_linter()
	)

[maelle]

Another question for @MichaelChirico and other lintr authors like @Bisaloo. And for @mpadge too!

In goodpractice, failures for a linter are presented to the user using a phrasing like

It is good practice to

  ✖ omit trailing semicolons from code lines. They are not needed and most R coding standards forbid them

    [R/semicolons.R:4:30]
    [R/semicolons.R:5:29]
    [R/semicolons.R:9:38]

That justification does not exist as is for most linters in the manual pages of linters. Would it be useful there? If it were, I could start a PR adding a justification at least for the linters we want to include in goodpractice. In the manual page of each linter, it'd exist as a section called "Why?" for instance, which might be useful for lintr users. And this way, in goodpractice we could lift what would be the official explanations.

[maelle]

The explanation could be very succinct and link to an external reference when relevant (for instance, for style rules, the tidyverse style guide).

[maelle]

This linter has a good justification: https://lintr.r-lib.org/reference/comparison_negation_linter.html, as do many of the newer ones actually.