Standardize R Code Format#19
Conversation
- remove these top level files/directories from being built with the package
d33bs
left a comment
There was a problem hiding this comment.
Nice job! I left a few comments and thoughts about things. These all seem like positive changes for the project so I'm approving pending your thoughts about revisions, cheers!
I have a good feeling you were conscious of tests based on the changes here but wonder openly if we should prioritize adding continuous testing through GH Actions (perhaps nearby the merge of this PR). This might help reassure maintainers that the changes here don't impact existing functionality which already has tests. I don't have a firm grasp on whether or how we should run tests to check the work for the project - could I ask for input on this @msubirana (and a double check with regards to this aspect)?
Completely optional ramblings about pre-commit:
I recognize you made some design choices to avoid pre-commit and wanted to contextualize those with some thoughts here. It might be possible to reduce the amount of code necessary to perform formatting or linting checks and fixes by using pre-commit as an optional development tool and a required CI check. One example of why we might opt to do this would be that someone doesn't want to use it as a git hook or forgets to manually run it using pre-commit run -a (which allows you to run checks without it being integrated into your git workflow).
Rough example of implementation:
- Add the file
.pre-commit-config.ymlwhich includes specific linters along with their versions. - Add a step to a workflow like this one to run
pre-commitwhich will check the work against the linters in thepre-commit-config.ymlfile. - Add a step which runs
pre-commit litethat can automatically update PR's when checks fix files automatically.
Here's an example pre-commit workflow for this which would need a few minor changes.
The benefit might be that multiple kinds of linting checks and fixes may be applied throughout the project (R or otherwise, for example markdown, yaml, etc.) and we don't need to maintain git code within CI (instead relying on pre-commit lite to do this type of work for us).
There was a problem hiding this comment.
This comment applies to the new linting/formatting additions generally and not just this file. How could we help prepare users who might want to use styler or lintr to check their work prior to a PR? Consider adding a CONTRIBUTING.md file or a relevant vignette Rmd file to help describe how someone could check or format their work within a local development environment. This might also be a great way to introduce how these checks apply to the project (i.e. We require all changes to pass lintr and styler checks in order to merge...).
There was a problem hiding this comment.
Agreed, having a contributing guide would be helpful. In general, I think it is OK to rely on the GHA to apply style changes, but it might be worth specifying that developers attest to following the guidance of lintr::lint_package() prior to submitting a PR.
…intended use and functionality of the GHA References: #19 (comment) Co-authored-by: Dave Bunten <d33bs@users.noreply.github.com>
- use `grep -E` instead of deprecated `egrep` - quote $FILES_TO_COMMIT to gracefully handle filename spaces, add xargs to improve handling of multiple files - use environment varialbe for github.head_ref References: #19 (comment) Co-authored-by: Dave Bunten <d33bs@users.noreply.github.com>
References: #19 (comment) Co-authored-by: Dave Bunten <d33bs@users.noreply.github.com>
a5e29af to
6531757
Compare
This PR addresses #11 by introducing consistent Bioconductor-compliant styling across the package and integrating tools to help maintain that style going forward.
Summary of Changes
Initial Styling Applied
The package has been reformatted using
styler::style_pkg(transformers = biocthis::bioc_style())to conform to Bioconductor style guidelines.Linting Configuration Added
A .lintr configuration file is included to support local use of
lintr::lint_package(). This enables developers to check for style issues and maintain consistency during development.Automated Style Enforcement via GitHub Actions
A GitHub Actions workflow has been added that automatically applies the Bioconductor styling rules via
styler::style_pkg(transformers = biocthis::bioc_style()).While a pre-commit hook configuration could offer even earlier enforcement, it may be overly burdensome for developers who prefer to push work-in-progress code without being blocked by style checks. That said, open to collaborating on a pre-commit setup if others feel it would be helpful!