Feature : Automated linting for files

[Sweetdevil144]

Description

PRIORITY : LOW/OPTIONAL

Is your feature request related to a problem? Please describe.
As our codebase grows, maintaining a consistent style and avoiding common coding issues becomes increasingly important.

Proposed Solution

Describe the solution you'd like
I propose that we use the lintr and styler packages in R to check for and fix common style issues. We can create a linter.R script that users can run from the terminal to check their code and fix issues.

Here's a basic example of how we can use lintr to check for issues:

# Install the lintr package
if (!require(lintr)) {
  install.packages("lintr")
}

# Check all R files in the current directory and its subdirectories
lintr::lint_dir(path = ".", pattern = "\\.R$", recursive = TRUE)

And here's how we can use styler to automatically fix some style issues:

# Install the styler package
if (!require(styler)) {
  install.packages("styler")
}

# Style all R files in the current directory and subdirectories
styler::style_dir(
  path = ".", 
  recursive = TRUE, 
  filetype = "R", 
  transformers = styler::tidyverse_style(scope = "tokens")
)

Alternatives Considered

Describe alternatives you've considered
An alternative would be to manually review the code for style issues. However, this can be time-consuming and error-prone. Automated linting and styling tools can help us maintain a consistent style with less effort.

Additional Context

This is an effort to improve the quality and maintainability of our R codebase. By enforcing a consistent style and catching common issues, we can make the code easier to read with less blue underlines :-)

[mdietze]

A while back Ben Bond-Lamberty did a lot of PRs where he did this sort of code cleaning package-by-package. While most of the automated checking was useful, some of it was not and had to be manually reverted. I'd worry that an automated linting would just reintroduce these mistakes in every single PR.

I'll also note that GH itself is now doing a lot of these sort of automated tests -- code with issues is showing up in PRs now outside of the code that individuals have changed.

[infotroph]

Preserving comment from @mdietze in #3407 that seems relevant here:

Be aware that the last time someone tried to apply style rules to the entire PEcAn repo, there were a fairly large number of places where manual reversions to the original code were required. I'm not up for having to fight an automated stylizer on every PR (or even periodically). I also want to make sure anything that gets implemented doesn't increase the bar of frustration that new members of the team feel when trying to learn how to submit successful PRs (too much time is already spent making changes that are not in bits of code the individual touched)

[infotroph]

My take: In principle I support using automation to get and keep the code uniformly formatted. But like @mdietze I do not want to fight with the stylizer. I do think the available linting/styling tools are better now than they were last time we tried this, but I think getting all the way to "no fighting" means we need all the following nontrivial components:

1. We need full group buy-in to the concept that code style is actively enforced and any time any of us disagree with the style robot, the only tenable solution is to let the robot win the argument. We are an opinionated bunch and this is harder than it sounds!
2. The automation needs to be complete and unobtrusive - we already have too many steps for users to remember or bounce off when they fail unexpectedly. I think this probably means automated styling of every changed file via GitHub actions after the PR is opened, rather than asking users to run it on their end.
   • which implies the styling happens after the user tests their code, so we need to really very thoroughly trust the styler not to change any code behavior.
   • which in turn limits the set of changes we'll be comfortable making. I think it means we're talking mostly about whitespace changes and not getting the benefit of more syntax-aware lints (like the one that saves me often: "1:length(...) is likely to be wrong in the empty edge case. Use seq_along(...) instead.")
3. It should be possible (and hopefully easy) for users to configure their own environment in a way that isn't in conflict with the imposed style -- I don't want to be "fixing" something my editor underlines only for the robot to change it right back. At the same time I hope this can be done in a project-specific way without forcing all contributors to change their global editor settings. Our house style should not dictate how anyone's non-PEcAn scripts are allowed to look.

[dlebauer]

I think putting a script in the scripts folder as in #3413 that is available for anyone who wants to lint / style their code is helpful. And it doesn’t require anything. It will be available to clean up code if wanted.

I agree that running it over the entire codebase may be more trouble than it is worth. But having it as an option can be useful.

One issue we should avoid though is having functional changes in the same commit pr as formatting changes. So yes, it does add additional work. Maybe the script could run a git commit to isolate its changes.

[infotroph]

I think putting a script in the scripts folder ... is helpful

Having scripts available for folks who want them is fine and low-cost, but it doesn't address any of the needs I listed above. Building on my point 3, let's remember that {styler} and {lintr} already exist and are integrated into the editors/IDEs many contributors use already, so an approach that works with those instead of against them is much more likely to succeed.