Cleaning lints across easystats packages

[IndrajeetPatil]

Preamble

Lints are code quality issues that can be detected in R using {lintr} and need to be either fixed (if it's really an issue) or annotated so that they are ignored. We detect lints using GHA workflow, and every package includes this workflow. Static code analysis is not always correct, esp. in edge cases, and so you should also see if the lints false positives. If yes, let me know, and I can report and try to fix this in lintr itself.

Example

What does lint output look like?

For an example, {insight} lint workflow currently shows the following lint (alongside many others):

Warning: file=R/check_if_installed.R,line=123,col=7,[outer_negation_linter] !all(x) is better than any(!x). The former applies negation only once after aggregation instead of many times for each element of x.

The code in question is:

  if (any(!x)) {
    if (any(x)) {
      cat("\n\n")
    }
    cat("Following packages are not installed:\n")
    print_color(paste0("- ", names(x)[!x], collapse = "\n"), "red")
  }

As the lint message says, this code can be rewritten to the equivalent and more efficient version:

  if (!all(x)) {
    if (any(x)) {
      cat("\n\n")
    }
    cat("Following packages are not installed:\n")
    print_color(paste0("- ", names(x)[!x], collapse = "\n"), "red")
  }

Once you make this change, the lint workflow will no longer flag these lines of code.

Help wanted

Although Daniel and I have been cleaning lints once in a while, this requires a much more concerted effort from the larger team since there are a ton of lints to be cleaned across repos.

I am tagging this issue with "Help wanted" and "Beginner-friendly" labels, hoping that we might get some community help to do this clean-up.

[IndrajeetPatil]

I have also updated .lintr files in all our repos to be in sync with the GHA workflow.

So, if you want, you can also run the linters locally (lintr::lint_package()), and you should see similar results to GHA.

[etiennebacher]

So, if you want, you can also run the linters locally (lintr::lint_package())

Note that you'll need the development version (r-lib/lintr)

[IndrajeetPatil]

Yes, as a good practice, always work with the development version of lintr to get the best and the latest.

[strengejacke]

This lintr-expression unnecessary_concatenation_linter(allow_single_expression = FALSE), is causing problems.

[etiennebacher]

You need the dev version of lintr, this function was called unneeded_concatenation_linter() before

[IndrajeetPatil]

Daniel has already removed that particular linter from config files.

[strengejacke]

Short question, what does the "test-example-coverage" workflow do? It's always failing, and I can't see the benefits from this workflow.

[IndrajeetPatil]

Short question, what does the "test-example-coverage" workflow do? It's always failing, and I can't see the benefits from this workflow.

See #331 for full details.

[IndrajeetPatil]

Okay, so in order to make this task more manageable, we can rely on The Boy Scout Rule:

Leave your code better than you found it.

A way to do this would be to clean lints only in the files that have changed in a given PR.

For example, if a given repo has over 200 files, but the PR has changed only 5 files, the GHA workflow for linting will fail only if the lints in these 5 files have not been cleaned. I think this is a much more manageable task than cleaning lints across all 200 files. If we do this for a few months, we should have made a significant headway on this task.

So, if you make a PR, watch for the build failures due to lint-changed-files workflow. This means that you haven't cleaned the lints in the files you have touched in the given PR.

Once again, if you can't figure out why something is producing a lint or how to get rid of it, let me know. Don't remove the workflow to make your life easier. That's just going to lead to us acquiring more technical debt.

[strengejacke]

_{this one is to torture @DominiqueMakowski}

[rempsyc]

report should be good now

[IndrajeetPatil]

@rempsyc is on fire! 🔥🔥🔥

Suddenly, {report} is competing with {effectsize}. 😅