-
Notifications
You must be signed in to change notification settings - Fork 0
Add GHA checks #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add GHA checks #11
Conversation
* call checks from {ghactions4r} for building and
updating pkgdown site, checking documentation
and style, adding a PR checklist, running R cmd
check, checking spelling
* copy GHA workflows from FIMS to adding all
contributors and greetings
* develop a reusable workflow update-data-r.yml
for updating data files in the package when
changes are made to the /data-raw
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds GitHub Actions workflows to automate various checks including building and updating the pkgdown site, documentation updates, automated data updates, style checking, and contributor acknowledgments.
- Introduces multiple GitHub Actions workflow files for tasks such as updating data files, running spell checks, and performing R CMD check.
- Updates documentation files and minor code comments to improve clarity and consistency.
- Includes configuration changes in pkgdown, DESCRIPTION, and the development container to support the new workflows.
Reviewed Changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkgdown/extra.css | Adds an external CSS import to support the pkgdown site style. |
| pkgdown/_pkgdown.yml | Configures site URL and sets Bootstrap version for the pkgdown site. |
| man/load_csv_ewe.Rd | Fixes a documentation typo in the parameter description. |
| man/get_functional_groups.Rd | Corrects the reference from “EeW” to “EwE” in the documentation. |
| man/ewe_nwatlantic_base.Rd | Updates the documented column count to 10 to reflect recent changes. |
| inst/WORDLIST | Introduces a wordlist to support spell checking. |
| data-raw/ewe_nwatlantic.R | Updates function calls to use the ecosystemdata namespace. |
| README.md | Adds a contributors section using an automated allcontributors setup. |
| R/load_.R | Revises inline comment text for clarity. |
| R/get_functional_groups.R | Updates a comment to correctly refer to the EwE model. |
| DESCRIPTION | Adds a language specification. |
| .github/workflows/*.yml | Adds multiple workflows for checks, documentation, and updates. |
| .devcontainer/devcontainer.json | Updates container packages to include the spelling package. |
| .Rbuildignore | Expands ignored paths to include new config and documentation directories. |
Comments suppressed due to low confidence (1)
man/ewe_nwatlantic_base.Rd:12
- Please verify that the updated documentation reflecting 10 columns accurately matches the actual structure of the data output.
10 columns:
| @@ -1,22 +1,22 @@ | |||
| # code to prepare `ewe_nwatlantic` datasets | |||
|
|
|||
| # TODO: remove this line after testing | |||
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing or resolving the temporary testing comment on line 2 before merging to avoid leaving development artifacts in production code.
| # TODO: remove this line after testing |
|
@kellijohnson-NOAA, most of the GHA workflows are using the reusable workflows provided by the {ghactions4r} package. The two newly added workflows are |
|
@k-doering-NOAA, I’ve added two workflows ( This workflow will also be needed in two other NOAA-FIMS repos. Do you think this could be a candidate for a reusable workflow in the {ghactions4r} package, or would it make more sense to keep it scoped within NOAA-FIMS? |
| pr-message: > | ||
| Thank you for contributing to {ecosystemdata} and opening your first PR | ||
| here! We are happy to have your contributions. Please ensure that the | ||
| PR is made to the dev branch and let us know if you need any help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PR is made to the dev branch and let us know if you need any help! | |
| PR is made to the main branch and let us know if you need any help! |
Right now, we are using the main branch for this repo rather than a dev branch.
@Bai-Li-NOAA , in my mind, this does seem in scope for ghactions4r, as it seems general enough to apply to many R packages. Do you want to submit a PR for that workflow there at some point? Or, we can open an issue for one of us to work on in the future. |
|
@k-doering-NOAA, I can submit a PR to {ghactions4r} to add a workflow for updating data files. We can then call the workflow from NOAA-FIMS. |
kellijohnson-NOAA
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. I have a few questions but nothing is blocking. Additionally, all files should end with an empty line, i.e.,
It is recommended that YAML files, like other text files, end with a single newline character. This convention is rooted in the POSIX standard for text files, which defines a text file as a sequence of lines, each terminated by a newline character.
| }, | ||
| "ghcr.io/rocker-org/devcontainer-features/r-packages:1": { | ||
| "packages": "dplyr,devtools,ggplot2,jsonlite,methods,mockery,Rcpp,RcppEigen,scales,snowfall,TMB,tibble,tidyr,usethis", | ||
| "packages": "dplyr,devtools,ggplot2,jsonlite,methods,mockery,Rcpp,RcppEigen,scales,snowfall,spelling,TMB,tibble,tidyr,usethis", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need Rcpp, RcppEigen, and TMB in this repository?
| name: Collect contributors | ||
|
|
||
| on: | ||
| push: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to run this on every push or at some other interval like monthly?
| ## Checklist | ||
| - [ ] The PR is requested to be merged into the appropriate branch (typically dev). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall having a dev branch in this repo.
| - [ ] The PR is requested to be merged into the appropriate branch (typically dev). | |
| - [ ] The PR is requested to be merged into the appropriate branch (typically main). |
| - [ ] The PR is requested to be merged into the appropriate branch (typically dev). | ||
| - [ ] The code is well-designed. | ||
| - [ ] The functionality is good for the users of the code. | ||
| - [ ] Code coverage remains high, indicating the new code is tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have code coverage in this repository?
| pull_request: | ||
| branches: | ||
| - main | ||
| push: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think will lead to two workflows on the same code so I recommend removing lines 8--10.
| # Update data files in /data whenever changes are made to files in /data-raw. | ||
| # This workflow automatically commits changes directly to the working branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is brilliant! I do not understand though why we are calling another file within this repository rather than just using the other file?
| r_scripts <- list.files("data-raw", pattern = "\\.R$", full.names = TRUE) | ||
| if (length(r_scripts) > 0) { | ||
| for (script in r_scripts) { | ||
| system(paste("Rscript ", shQuote(script))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused 😕 why you would use a system call to run Rscript instead of just sourcing the function within R here?
|
|
||
| usethis::use_data(ewe_nwatlantic_base, overwrite = TRUE) | ||
| usethis::use_data(ewe_nwatlantic_env, overwrite = TRUE) | ||
| usethis::use_data(ewe_nwatlantic_env, overwrite = TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a new line at the end of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:nit: I am not sure what is best practice but I prefer that the list just be alphabetical not not sorted by capitalization first.
What is the feature?
How have you implemented the solution?
updating pkgdown site, checking documentation
and style, adding a PR checklist, running R cmd
check, checking spelling
contributors and greetings
for updating data files in the package when
changes are made to the /data-raw
Does the PR impact any other area of the project, maybe another repo?
TO-DO: