Skip to content

Commit

Permalink
Removes some sections in 07
Browse files Browse the repository at this point in the history
* Removes "Roadmap to FIMS File Structure and Organization" info will be put in README files in each folder eventually, see NOAA-FIMS/FIMS#650
* Removes section on "Merge Conflicts" given the 3-1 vote for removing it in #144
* Removes section on "GitHub Actions Results" because it is a button that is visible from the top of the repository and this information is not specific to FIMS
* Removes "GitHub Teams" because we are not using them
* Moves "Commit Messages" to style guides
* Remove hard-coded wrap on lines
  • Loading branch information
kellijohnson-NOAA authored Sep 19, 2024
1 parent 53d2804 commit 39e4d98
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 67 deletions.
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

0 comments on commit 39e4d98

Please sign in to comment.