Reviewer guide#

Prerequisites#

Familiarity with the process of reviewing pull requests (PRs) on GitHub is assumed. This page is intended to provide guidance specific to the review process for ANTS and related repositories.

See also

About pull request reviews

The GitHub documentation provides general guidance on reviewing pull requests.

Reviewer considerations#

The reviewer should consider at least the following items:

  1. Does the pull request (PR) address the issue it is meant to?

  2. Are the PR summary and template completed?

  3. Is the information on the PR sufficient? This may involve iterating with the developer on understanding changes.

  4. Are the code conventions followed?

  5. Is any AI attribution self-consistent (i.e. both commit messages and file headers where needed), and consistent with policy (i.e. only Met Office GitHub Enterprise for Met Office contributors).

  6. The core and/or ancillary-file-science (as appropriate) rose tests should be re-run by the reviewer to avoid “works for me and only me” type issues (e.g. due to settings in ~/.bashrc).

  7. Have the documentation changes been checked in the built documentation? (Does not apply to ancillary-file-science)

  8. For ANTS PRs, does there need to be a corresponding UG-ANTS PR (and vice versa)?

  9. For large code changes, consider running pytest coverage on the branch and on main to evaluate if the testing coverage is sufficient. The command is:

    python -m pytest --cov=ants .

    python -m pytest --cov=ugants .

  10. Have the github actions passed as expected?

  11. Are there any paths present in the code that need removing before merge to main?

  12. If you are using github’s suggest changes feature, have you signed the CLA? If a change is accepted from a suggested change you become an author on that change so need to have signed the CLA.

  13. Have you double checked the labels and milestones for the PR are correct?

  14. Is the PR title informative as to what the PR is doing?

  15. In some cases new Apps may be added to ug-ancillary-file-science without the need to add rose-stem apps. They should still have tests added as part of the PR. Have you checked these tests have been added to the unittests group in rose-stem?

  16. If a new App has been added, have you checked that the tests from that app are being run in the github actions?

  17. Does the change need examining by an ancillary science owner or ancillary workflow owner for testing/validation as part of the review process? Examples would be for changes that affect numerical results or add new science routines.

If there need to be changes to the environment, a developer environment named developer-NNNN (for PR NNNN) should be deployed centrally using the standard process. This environment should then be used for the testing. The need for the centrally installed environment is that we need a check that the tests pass with an environment that can be created centrally, rather than relying on an individual developer’s environment.

Review comments#

Please use the GitHub “Start a review” or “Add single comment” options to create new comments during review iterations, rather than editing previous comments. This ensures that the conversation history is transparent for future readers and that other people on the PR are notified of changes.

Tip

For small one-line-changes or typo fixes, reviewers may use the GitHub “suggest changes” feature. This can speed up the review process, as the developer can choose to apply the suggested changes straight to their branch. Do not use this feature for significant changes, for example adding new code or restructuring existing code.

See also

Commenting on a pull request

The GitHub documentation provides general guidance on commenting on pull requests, including how to suggest changes.