Questions: Do developers need to update documentation locally and check in updates?

[Bai-Li-NOAA]

Hi @k-doering-NOAA ,

@Andrea-Havron-NOAA brought up a great question regarding call-doc-and-style-r.yml in the pull request #54. When working on a feature branch, do developers need to run devtools::check(), or devtools::document(), or roxygen2::roxygenise() locally and then check in updated documents (e.g., NAMESPACE)? I noticed that FIMS runs call-doc-and-style-r.yml on main branch. I wonder if we need to run the workflow on feature branches as well and add NAMESPACE to .gitignore, so developers do not need to check in updated documentation to the repo. Thanks!

[k-doering-NOAA]

Good question, @Bai-Li-NOAA ! The way I think of the github actions are as back up.

developers are welcome to update documentation by running devtools::document() locally; they are also welcome to run styler locally and push changes (although best to do this in at least a separate commit if styler is making a lot of changes).

As for devtools::check(), that is also great for developers to run locally, but if they don't, the github action serves as back up (I admit, I am guilty of relying on github action to check for me).

I'm concerned things could get confusing quickly if we automatically style and document on the feature branches. We could potentially style and document pull requests before they are merged in, but merging in changes to main might still require another style and document run.

If we do eventually have branches that we expect users to be trying out (e.g., separate main and dev branches), I do think it makes sense to style and document both of these.

A little off topic, but I also wonder what you and Andrea think about the style and document job opening a PR? I think we could instead set it up to automatically commit to main (although I'm not sure if this will work or not with branch protection rules). This would mean style and document would just happen without a developer needing to accept the PR.

Does this all make sense? My opinion may differ from others.

[Bai-Li-NOAA]

Yes, it makes sense. It might be easier to continue using the current workflow, so developers run devtools::document() locally, check in changes, and the GHA workflow runs as back up on the main branch. Otherwise, I can see that there will be many automated PRs sitting in the feature branches.

I think pre-commit hook is another option, which forces developers to inspect the code before pushing code to the feature branches. It will not create a lot of pull requests, but forces developers to check code more often locally. However, I do not know how to set up those hooks yet. Definitely on my to-learn list.

[k-doering-NOAA]

Yes, the pre-commit hook is a cool idea! I think the issue is that it will need to be set up locally on every computer the developer is using - I also wonder what happens if someone just edits files right on github (I sometimes do this for small changes)?

Anyway, I don't know much about them either, but they do seem a useful supplementary tool should developers want to use them (esp. for checks like pushing secrets)! I think if they are relatively easy to set up, perhaps it is worth the effort to get the core FIMS developers using them!

[Bai-Li-NOAA]

Great point on local setups. Will use ghactions4r as a testbed to explore if we need to use pre-commit hook for FIMS. For small changes, we may can rely on back up GHA on the main branch 😄

Based on the discussion, seems that we could add a section in the FIMS developer handbook and provide instructions on what can be done locally before making a PR (if we are not using GHA to create pull request on feature branches).

• run cmake build and ctest locally and make sure the C++ tests pass
• run devtools::document() locally and push changes to remote feature branch
• run styler::style_pkg() locally and push changes to remote feature branch (best to do this in at least a separate commit if styler is making a lot of changes)
• run devtools::check() locally and make sure the package can be compiled and R tests pass. Run devtools::test(filter = "file_name") (where "test-file_name.R" is the testthat file containing failing tests) to troubleshoot tests if they are the reason devtools::check() isn't passing.

Feel free to edit the list!

[k-doering-NOAA]

This sounds like a great list. I can't think of anything else R related to suggest, except that we could explain that if tests are failing, devtools::test(filter = "file_name") could be used to troubleshoot test if they are the reason devtools::check() isn't passing.

Maybe it would be good to add a developer checklist to the PR template that includes these steps, in addition to providing instructions in the handbook? That way, the developer doesn't have to go looking for it when they want to submit a PR, it is just there.

[Bai-Li-NOAA]

Adding a checklist to the PR template sounds good! I added devtools::test(filter = "file_name") to the list and will work on providing instructions in the handbook. @k-doering-NOAA, please feel free to add the list to the FIMS PR template or I can make the changes and then request a review from you! Thanks!

[Andrea-Havron-NOAA]

I've come across a couple additional checks that are required before submitting to rOpenSci:

1. pkgcheck() from the pkgcheck library
2. autotest_package() from the autotest library

We may or may not want/need to submit FIMS to rOpenSci, but the checks are still useful in detecting potential problems or inefficiencies in a package.

[JaneSullivan-NOAA]

On the issue of auto-populating the NAMESPACE file based on Roxygen documentation: Until TMB is updated, we cannot compile the FIMS package. Therefore we updated the R/io.R documentation section to include the FIMS registration and Rcpp evaluation. See the documentation-update-io.R branch for changes. Can't test that this fixes the issue until compilation issue is fixed. At that point, merge documentation-update-io.R into main branch.

[k-doering-NOAA]

I think this has been resolved, but feel free to reopen if there is some part that needs addressing.