Skip to content
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

Rework 07 and 11 #157

Merged
merged 3 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions 02-FIMS-Project-Overview.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ Run [`cmake --build build` and `ctest --test-dir build` locally](https://noaa-fi
and make sure the C++ tests pass before pushing tests to remote feature branch. If there are failing tests, run `ctest --test-dir --rerun-failed --output-on-failure` to re-run the failed tests verbosely.
* Ensuring the run-clang-tidy and run-googletest [Github Actions workflows](#github-actions) pass on the remote feature branch

Below is a list of online resources for learning C++:
* [cplusplus.com](https://cplusplus.com/doc/)
* [docs.microsoft.com](https://docs.microsoft.com/en-us/cpp/standard-library)
* [geeksforgeeks.org](https://www.geeksforgeeks.org/c-plus-plus/?ref=outind)
* [learncpp.com](https://www.learncpp.com/cpp-tutorial)
* [positioniseverything.net](https://www.positioniseverything.net/category/coding/c/)
* [w3schools.com/cpp](https://www.w3schools.com/cpp)

### R developers

The R developers responsibilities include:
Expand Down
75 changes: 9 additions & 66 deletions 07-contributor-guide.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@ Style guides are used to ensure the code is consistent, easy to use (e.g., read,
We use the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html) for C++ code.

#### R

We use the [tidyverse style guide](https://style.tidyverse.org/) for R code.

#### Naming Conventions

We use `typename` instead of `class` when defining templates for consistency with the `TMB` package. While types may be defined in many ways, we use `Type` instead of T` to define Types within FIMS.
We use `typename` instead of `class` when defining templates for consistency with the `TMB` package. While types may be defined in many ways, we use `Type` instead of `T` to define Types within FIMS.

#### Commit Messages

Commit messages communicate details about changes that have occurred to collaborators and improve team efficiency. The best guidance for how to create an excellent commit message can be found on [Conventional Commits](https://www.conventionalcommits.org), which is our style guide for commit messages.

### Coding Good Practices

Expand All @@ -38,42 +43,6 @@ Following good software development and coding practices simplifies collaboratio
* Limit line length (wrap ~72 characters) of code but use a single line per paragraph in this markdown document
* Capitalize SQL queries so they are readily distinguishable from table/column names

## Roadmap to FIMS File Structure and Organization

### Files that go in `inst/include`

#### common

This folder includes files that are shared between the interface, the `TMB` objective function, and the mathematics and population dynamics components of the package.

#### distributions

TODO: document distributions folder.

#### interface

This includes the R interface files.

#### population_dynamics

Each folder in population_dynamics corresponds to a component of the population dynamics model. Given the complexity of the component, the structure in these folders within population_dynamics may differ. At a minimum there will be a `.hpp` file with the same name as the subfolder, e.g., fleet/fleet.hpp. This file will include an `ifndef` directive and `#include` statements. If the folder is empty other than this file, then the remainder of the file will define the component. If there are additional subdirectors along side the .hpp file, the file will end after the include statements that point to each of the files in the functions folder that sits next to this .hpp file.

For the latter scenario, where a functor folder exists, inside the functor folder will be a `.hpp` file with `_base` attached to the component name, e.g., `population_dynamics/maturity/functors/maturity_base.hpp`. This `_base.hpp` file will define the base class for the module type. The base class should only need a constructor method and a number of methods (e.g., `evaluate()`) that are not specific to the type of functions available under the subfolders but reused for all objects of that class type.

#### utilities

TODO: document utilities folder.

### Files that go in `src/`

#### FIMS.cpp

This is the `TMB` objective function.

#### Makevars

TODO: document makevars and makevars.win files

## GitHub Collaborative Environment

Communication is managed via the [NOAA-FIMS](https://github.com/NOAA-FIMS) GitHub organization.
Expand Down Expand Up @@ -175,20 +144,12 @@ If a review has been assigned to you and you don't feel like you have the expert

##### Review Checklist

While automated testing can assure the code structure and logic pass quality checks, human reviewers are required to evaluate things like functionality, readability, etc. Every PR is accompanied by an automatically generated [checklist of major considerations for code reviews](https://github.com/NOAA-FIMS/FIMS/blob/main/.github/workflows/pr-checklist.yml). Additional guidance is provided below for reviewers to evaluate when providing feedback on code:
While automated testing can assure the code structure and logic pass quality checks, human reviewers are required to evaluate things like functionality, readability, etc. Every PR is accompanied by an automatically generated [checklist of major considerations for code reviews](https://github.com/NOAA-FIMS/FIMS/blob/main/.github/workflows/pr-checklist.yml).

##### Reviewer Good Practices

Good reviews require good review habits. Please use the guidance found at [Conventional Comments](https://conventionalcomments.org/) for formatting/structuring your reviewer comments. Navigate to the [Perforce Blog](https://www.perforce.com/blog/qac/9-best-practices-for-code-review) for nine best practices for code review and use the [FIMS Style Guide](#style-guide) to settle any style arguments.

#### Merge Conflicts

[Merge conflicts](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/about-merge-conflicts) must be resolved before a PR can be merged into the desired branch because Git cannot automatically determine which version of the code should be kept.

Merge conflicts can be resolved [on GitHub](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-lineithub.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/about-merge-conflicts) or [locally using Git](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line).

Tools such as [Git Kracken](https://www.gitkraken.com/learn/git/tutorials/how-to-resolve-merge-conflict-in-git) or [GitLens within VS Code](https://marketplace.visualstudio.com/items?itemName=eamodio.gitlens) offer a GUI interface to help resolve merge conflicts. [Tatiana Tylosky's guide to merge conflicts](https://github.com/TatianaTylosky/solving-and-preventing-merge-conflicts/blob/master/guide.md) is also helpful.

### GitHub Actions

FIMS uses [GitHub Actions](https://docs.GitHub.com/en/actions) to automate routine tasks such as testing, building and deploying websites, and notifying individuals. Some of the actions are built on [reusable workflows](https://docs.GitHub.com/en/actions/using-workflows/reusing-workflows) available in [{ghactions4r}](https://nmfs-fish-tools.github.io/ghactions4r/), where reusable workflows allow for sharing of code.
Expand All @@ -201,10 +162,6 @@ For the repositories in NOAA-FIMS that use GitHub actions, the actions can be fo
- **run-clang-tidy** runs checks while compiling the C++ code. If this run fails, fixes need to be made to the C++ code to address the issue identified.
- **run-googletest** runs the GoogleTest C++ unit tests and benchmarking. If this run fails, then fixes need to be made to the C++ code and/or the GoogleTest C++ unit tests. To replicate this GitHub Actions workflow locally, follow instructions in the [testing section](#testing).

#### GitHub Actions Results

Completed instances of the GitHub Actions can be viewed by navigating to the Actions tab a repository. For example, the [NOAA-FIMS/FIMS/actions](https://github.com/NOAA-FIMS/FIMS/actions). The status of GitHub Actions applicable to a PR can be viewed on the GitHub PR after the PR is initiated or after any commit is pushed to a PR.

#### Debugging Broken GitHub Actions

GitHub Actions can fail for many reasons, so debugging is necessary to find the
Expand All @@ -213,19 +170,9 @@ cause of the failing run. The tips found below can be helpful for determining wh
- Ask for help as needed! Some members of the FIMS team who have experience debugging GitHub Actions are Bai, Kathryn, and Ian.
- Investigate why the run failed by looking at the log file. GitHub provides some guidance on what is contained in the [log file](https://docs.GitHub.com/en/actions/quickstart#viewing-your-workflow-results).
- Try to replicate the problem locally. For example, if the call-r-cmd-check run fails during the testthat tests, try running the testthat tests locally (e.g., using `devtools::test()` in an R session).
- If the problem can be replicated, try to fix locally by fixing one test or
issue at a time. Then push the changes up to GitHub and monitor the new GitHub
Action run.
- If the problem can be replicated, try to fix locally by fixing one test or issue at a time. Then push the changes up to GitHub and monitor the new GitHub Action run.
- Check if the problem is present across all investigated operating systems because it might be operating system specific and not reproducible on your machine. For example, if using Windows locally, it may be an issue specific to Mac or Linux.
- Sometimes, runs may fail because a particular dependency was not available at
the exact point in time need for the run (e.g., maybe R did not install because
the R executable could not be downloaded); if that is the case, wait a few hours
to a day and try to rerun. If it continues to fail for more than a day, a change
in the GitHub Action YAML file may be needed.

### GitHub Teams

[GitHub Teams](https://docs.github.com/en/organizations/organizing-members-into-teams) can help organize members into groups with each group having certain permissions. [Teams for NOAA-FIMS](https://github.com/orgs/NOAA-FIMS/teams) are not currently used.
- Sometimes, runs may fail because a particular dependency was not available at the exact point in time need for the run (e.g., maybe R did not install because the R executable could not be downloaded); if that is the case, wait a few hours to a day and try to rerun. If it continues to fail for more than a day, a change in the GitHub Action .yml file may be needed.

## git

Expand Down Expand Up @@ -272,7 +219,3 @@ $ git push origin <branch-name>
$ git checkout main
$ git branch -d <branch-name>
```

### Commit Messages

Commit messages communicate details about changes that have occurred to collaborators and improve team efficiency. The best guidance for how to create an excellent commit message can be found on [Conventional Commits](https://www.conventionalcommits.org), which is our style guide for commit messages.
2 changes: 1 addition & 1 deletion 10-testing.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Inside of your cloned version of FIMS, the file `CMakeLists.txt`, in the top-lev

### Build and run the tests

The following commands on the command line execute the outlined steps and are needed to build the tests:
The following commands on the command line (note that Windows users cannot use Git bash and must use a native Windows shell) execute the outlined steps and are needed to build the tests:

1. Generate the build system using `Ninja` as the generator, which creates the `build` directory.
1. Use `Cmake` to build in the `build` directory in parallel using 16 jobs but `--parallel 16` can be deleted.
Expand Down
56 changes: 0 additions & 56 deletions 11-glossary.Rmd

This file was deleted.

Loading