Policy for automating actions before commits

[pratikunterwegs]

This issue continues a discussion with @Bisaloo on automating actions that improve the quality of code. This mostly means improving code readability by formatting code according to a style guide. Any thoughts on this, including choice of style guide, are welcome here.

An example of the actions intended is running styler::style_pkg(): this can be done manually every so often. However, it could also be automated so that each file is styled before any changes to it are committed. This prevents code styling comments from cluttering a commit history.

One way of automating this process is by using git-hooks. An alternative, or extension, is a tools based on git-hooks such as pre-commit. A git hook could include a code formatting step, so that code is properly formatted before comitting.

[Bisaloo]

What kind of standards do we want to use?

I believe the tidyverse style is good.

I also like the data-driven approach in this recent R journal article: A Computational Analysis of the Dynamics of R Style Based on 108 Million Lines of Code from All CRAN Packages in the Past 21 Years. Spoiler: the style they recommend at the end, based on the most common style choices in CRAN packages, is equivalent to the tidyverse style.

(There is an interesting chicken and egg problem: has the tidyverse adopted the most popular style? Or is this style the most popular due to the success or the tidyverse? Or do we even have some kind of evolutionary convergence towards a common style?)

How to assess compliance?

• styler, which can fix the detected issues. Note that even though styler is a tidyverse tool, other styles are also implemented.
• lintr, which only annotates issues. However, lintr recently has added a long list of new linters that go much beyond just style issues. It now detects many potential bugs and inefficiencies in the code.

How to enforce these standards?

Local approaches

• Ask users to follow these standards using the tools of their choice. A good, cross-editor, way to share basic style guidelines is to use an editorconfig file specifying the size of indents, etc. It should be noted that RStudio has not implemented support for editorconfig just yet.
• Use the pre-commit approach to make sure code is compliant before committing

GitHub Actions approaches

I don't believe local approach can efficiently contribute to standard enforcement. It would require each contributor to set up the git hook on their local machine, which is not great in terms of scaling, and which doesn't offer us any guarantee these steps are actually running.

I believe a better approach is to use GitHub Actions to enforce these standards (although contributors can still be encouraged to check locally / set up a git hook). I'm split on whether automated workflows should automatically perform the changes (styler workflow) or only annotate issues and letting contributors fix the issues with the tools of their choice (lintr workflow). Automatically making the changes reduces the workload of contributors, but might create some confusion, and unnecessary merge commits, especially if pull.rebase has not been set to true, as recommended in #6.

I believe we will need to have a lintr workflow in all cases, because as mentioned just above, it contains many more tests than styler, and some of the detected issues cannot be fixed automatically.

This brings us to a follow up question: if we use automated styling via GitHub Actions, when should it run?

• on each push to PR targeting main?
• on each push on main? This should only be done for purely stylistic changes because we don't want to break anything already on main. This might require some tinkering because as mentioned by Thibaut in #6, it's not possible to run workflows with GITHUB_TOKEN on a protected branch.
• right before merging? It's not possible out-of-the-box to set GitHub Actions to run before merging but some projects have taken an approach where they don't merge themselves via GitHub web interface. They use GitHub Actions to do it instead. A merge could be triggered when a maintainer types /merge for example. It would then be easy to add extra steps before merging.