CodeReview

Requesting a review

Once a pull request (PR) is ready for review, it should be given the "ready for review" label and a review requested from one or more people with permission to review. (Only one review need actually be performed though.). Please ensure that the PR follows the reviewing guidelines below as these guidelines are used by the reviewer to determine whether the PR can be merged to master. Once a PR has been reviewed it will either be merged to master or returned to the requester for changes with a "reviewed with actions" label and a comment referencing the PR proposer.

Reviewing guidelines

1. Give the pull request the "under review" label instead of the "ready for review" to prevent race conditions!
2. Check that branch is up-to-date with master (the pull request should report that the branch can be "merged automatically"). If not, return to developer for them to bring it up-to-date.
3. Check all tests pass. This is done automatically by GitHub Actions.
4. Use the "Files changed" tab on the pull request to review all code changes. Check that all code modifications are as pythonic as possible, easy to understand and that the code is correct. Comments and requests for changes may be made in-line on the "Files changed" tab. This makes it easier for the developer to see which part of the code is being discussed.
5. Check that the interface to any new/modified routine/method is fully documented using Sphinx markup (see InterfaceMarkup). Note, some markup rules are: 1) Markup associated with a class init method should be put in the class docstring, 2) Documentation should be spell checked, and 3) Continued lines should have \ at the end of them.
6. If changes have been made to one or more implementation(s) of the fparser2 fortran2003 rules then, 1) check that the associated tests from the test_fortran2003.py file have been moved into a separate file in the fortran2003 subdirectory following the naming convention (test_<rule_name>_<rule_number>.py) and 2) ensure that the implementation of the rule(s) fully support(s) the Fortran2003 standard definition (by checking it with the standards document referenced in the documentation).
7. Check all modified/new code is covered (CodeCov runs automatically but you still need to check)
8. Code should be formatted following the Black style (this is checked automatically by a GitHub Action).
9. Check all modified/new code conforms to pylint
10. Check that any x-failing tests are unaffected by the current issue
11. Check that any new or modified x-failing tests or TODO comments reference an Issue.
12. Check documentation builds correctly
13. Check documentation is up-to-date if new functionality has been added in the Issue

Once the review is complete there are two options:

1. If the reviewer is happy with the pull request then they can proceed to merge the branch onto master (see below)
2. If the reviewer is requesting changes, then the "under review" label on the PR should be replaced by "reviewed with actions" and it is then up to the original developer to address the reviewer's concerns.

Merging a branch to master

Once a pull-request has passed code review, the code reviewer should merge the associated branch onto master:

1. Check-out the most recent version of the branch
2. Check that all tests pass in this version (sanity check)
3. Check that the documentation builds in this version (sanity check)
4. Update the changelog in the top-level directory (using the text describing the Issue)
5. Commit these changes and push branch to github
6. On the pull-request page on github, request the branch be merged to master
7. Checkout the latest master branch and check that all tests still pass
8. Delete the original branch
9. Close the associated issue