MPAS Ocean Review Guide

Reviewing MPAS-Ocean Pull Request

---

When reviewing a pull request to be merged into ocean/develop or ocean/private, please make sure the following are true:

1. You are not merging your own Pull request

2. The difference has been linted (This will catch the following errors) see instructions below about running the linter:

   1. No hard coded real values occur without the _RKIND specifier. i.e. 5.0 -> 5.0_RKIND.
   2. No lines have more than 132 characters (even comments lines)
   3. No lines have trailing white space
   4. The changes follow existing formatting (i.e. tabs vs. spaces and white space at the end of lines)
      • XML and C Files use tabs, please ensure this is always true. Formatting differences make debugging a registry file harder.
      • Fortran files use spaces

3. All mpas_pool_get_array and mpas_pool_get_field calls from a pool with multiple time levels have the time level argument. i.e. call mpas_pool_get_array(statePool, 'layerThickness', layerThickness) -> call mpas_pool_get_array(statePool, 'layerThickness', layerThickess, timeLevel).

4. The changes before and after the merge build with all of our supported compilers.

5. None of the commits in the history of the branch touch files outside of src/core_ocean (i.e. git log --graph --oneline --decorate --name-status HEAD only lists files in src/core_ocean).

   1. If a commit does touch files outside, the commit need to be removed. The change cannot be removed in a separate commit.

6. Commits have reasonable commit messages. They should all follow the format below. Specifically, the subject needs to be short, there needs to be a blank line on the second line.

7. All of the other cores still build.

8. Features of the ocean are still passing (i.e. bit-reproducibility across decompositions, and bit-restartability)

9. The PR is submitted with the proper format (Following ACME requirements here):

   1. The title of the PR is descriptive of the feature not just the branch name
   2. The description of the PR clearly describes what the PR is intended to do
   3. The pull request is submitted in the correct order (a feature branch should be merged into ocean/develop).

10. If you are reviewing a new test case in the testing infrastructure, the test case is expected to work with completely empty locations for any of the paths in the configuration file. This allows users on laptops to easily setup / use the test cases. If this is not possible, the developer needs to work with someone to fix the issue.

For new features:

1. The PR is accompanied by a design document (Preferably referenced in a comment within the PR, but not in the top description since that goes into the commit message).
2. The design document has gone through the proper review process

Commit message formatting:

All commit messages should follow this format.

subject (<= 72 chars)
<blank line>
body (however long it needs to be)

Merge commits are allowed to have longer subjects if the branch name forces it, but they should still be reasonably short, and the remainder of the message should follow the same format.

#Linting MPAS Source Files: The mpas_source_linter.py script is available in the MPAS-Tools repository.

An example of how it could be used when reviewing a pull request (or to clean up a pull request) is as follows:

Get a list of files that are modified or added:

We can use git diff to get a list of files that have been modified or added. We should ignore files that are deleted.

git diff HEAD MPAS-Dev/MPAS/ocean/develop --name-status | grep "^[AM]" | awk '{print $2}'

This list of files will be the files we need to lint.

Lint all files that are modified or added:

An example bash loop for linting all files that are modified or added is as follows:

for FILE in `git diff HEAD MPAS-Dev/MPAS/ocean/develop --name-status | grep "^[AM]" | awk '{print $2}'`; do ${LINT_SCRIPT} -f ${FILE}; done

After this is done, git status will show new files with the *.e extension, which contain the errors that were found when linting the file. It can be used to fix the errors in the specific file.

Cleaning up error files

There are many ways to clean up error files, you could manually remove them, or write a loop to remove them similar to the one for linting all files.

Git also provides a way to remove any ignored or untracked files.

git clean -fdx

can be used to delete all of the *.e files, but keep in mind, it will also delete any files that are untracked in the directory that are not *.e files.