Skip to content

Development Practices

Jump to bottom
Joe Wallwork edited this page Apr 17, 2024 · 21 revisions

This page documents highly recommended practices for contributions to Animate, Goalie, and/or Movement (referred to herein as "the Mesh Adaptation codes". Please read the page in its entirety before starting development work.

License

Animate, Goalie, and Movement have MIT licenses. Please familiarise yourself with their terms.

GitHub comments

  • If you make a comment anywhere in the GitHub API (e.g., on an Issue or PR) and realise it wasn't quite right, do not delete the comment. Many users have GitHub configured to send them emails and trying to track down a deleted comment from an email can cause a lot of confusion.
    • Instead of deleting the comment, edit it and use strikeout to delete the offending text. (To do this, put ~~ before and after the text you want to strike out.)
    • For extra clarity, add another comment starting with "Edit:", which explains that what you said was incorrect and why.

Code comments

  • Make sure that comments are used wherever it isn't obvious what a piece of code is doing.
  • However, do not add comments when it is obvious, e.g.
# Rename the function
f.rename("my_function")

Projects

Mesh Adaptation development board

The three Mesh Adaptation codes and their documentation repo have a shared Project board for ongoing development work. The project board has five categories:

  1. Backlog - Issues that have been flagged but there are not currently any plans to address them.
  2. Priority - Issues that have been flagged as important and should be addressed as soon as possible.
  3. In progress - Issues for which one or more developers are currently working on addressing.
  4. In review - Issues for which there is at least one open Pull Request that has reviewers assigned to it.
  5. Done - Issues that have been addressed.

Notice that all five categories refer to Issues. Pull Requests should not be put on the project board.

Other project boards

During the course of developing the mesh adaptation codes, it may be useful to set up new project boards, e.g., for sprints or hackathons.

Issues

  • If you identify a bug, missing feature, or other issue with one of the codes then raise and Issue for it:
  • Add labels to your Issue using the right-hand panel, for example identifying whether it relates to a "bug" or an "enhancement". If you think the Issue is of high priority then use the "PRIORITY" label, too.
  • Add your new Issue to the Mesh Adaptation development project board and set its status to "Backlog" once the Issue is created. If you used the "PRIORITY" label above then use that status here, too.
  • Set the milestone of the Issue to an appropriate upcoming milestone from the drop-down menu.
  • If you are happy to take on a particular Issue, add yourself to the list of assignees using the right-hand panel.
  • Every Pull Request should have an associated Issue. (Think of Issues like a ticketing system.)

Pull Requests

When you are ready to start work on an Issue, the recommended workflow is as follows.

  1. Command line: Check out the main branch (or whichever other branch you wish to start work from the head of).
  2. Command line: Run git pull to make sure you have the latest changes.
  3. Command line: Create a new branch with git checkout -b XX_<branch_name>, where XX is the Issue number and the branch name uses snake_case, e.g., 3_blind_mice.
  4. Command line: Whenever you make a change, commit it with a message starting with #XX, where again XX is the Issue number. This will provide updates to the Issue on progress that has been made (in the next step). Note that formatting checks may cause your commits to fail on the first attempt - see below for details.
  5. Command line: Once you are happy with your changes, push them to GitHub with git push. If this is the first time you have pushed your branch, you will need to set the remote, too, e.g., git push -u origin 3_blind_mice.
  6. GitHub: The first time you push the branch, a URL hint will appear for you to open a corresponding Pull Request (PR). Use that link (or the New Pull Request button) to open a PR.
  1. GitHub: Start the PR text box with "Closes #XX.", where XX is the Issue number. If your PR addresses multiple issues then replicate this for each of them. These statements create links within GitHub such that merging the PR will close the corresponding Issues. Fill the rest of the text box with details of what the PR sets out to do and how it achieves this.
  2. GitHub: In the right-hand panel, add yourself to the list of assignees and replicate any labels from the Issue. Do not set a Project - they are for Issues only.
  3. GitHub: Once you have pushed any changes required to make the Test Suite pass, assign one or more reviewer(s) (if in doubt of who to assign, put jwallwork23).
  4. GitHub: See below for details on the review process. If the reviewer(s) request changes, you will need to make edits to your branch and/or add comments to the PR to justify your approach. This will likely be an iterative back-and-forth process.
  5. GitHub: Once the reviewer(s) are happy with the changes, they will approve the changes and leave you to click the green "Merge pull request" button. DO NOT click this button until the PR has been approved. It is good practice to delete your branch using the purple button once you have done this. (Remember to delete your local copy of the branch, too.)

Review Process

Here we assume two parties: the developer(s) (who wrote the code in the PR) and the reviewer(s) (who have been assigned to review the PR). We also assume that the process outlined in the Pull Requests section above has been followed up to and including point 9. The recommended workflow for the reviewer(s) to follow is as follows:

  1. GitHub: Read through the PR description and any comments that have been left, replying if appropriate.
  2. GitHub: Check that all tests in the test suite have passed - the latest commit should have a green tick on the right-hand side. If there is a red cross then at least one test has failed.
  3. GitHub: Click the "Files changed" tab to view the code changes being proposed. You can leave inline comments on specific lines by clicking a specific line number or clicking and dragging over a range of line numbers. You can also leave comments on specific files using the speech bubble icon at the top of the changeset for that file.
  4. Command line (optional, but recommended): Check out the branch associated with the PR and run tests locally, for example any that have been mentioned specifically in the PR, any new demos introduced, and/or any other cases that you might be concerned about.
  5. GitHub: Once you are satisfied that you have made a decision about the PR, return to the "Files changed" tab and click the green "Review changes" button on the right-hand side. Choose to simply leave a comment (no conclusion yet), approve, or request changes for the PR and leave a summary comment justifying this choice.
  6. GitHub: If you did not approve the changes then there may be an iterative back-and-forth between the developer(s) and reviewer(s). In each iteration, follow the process outlined in points 1-5 above.
  7. GitHub: Upon approving the PR, DO NOT click merge - leave them to do that, as they might notice something they forgot at the last minute.

Testing

Animate, Goalie, and Movement each have dedicated test suites. All three have a subdirectory called test containing a mixture of unit tests and integration tests. Goalie also has a test_adjoint subdirectory, which includes tests for functionality making use of adjoint methods. (More accurately, the test subdirectory of Goalie includes test which do not make use of the adjoint, i.e., annotation is not turned on.)

Unit tests

The unit tests use Python's unittest module to check the core functionalities of the modules work as expected:

  • Check functions and methods are usable.
  • Check that, given some input values, functions and methods produce the expected output values.
  • Check exceptions are raised in the cases where they would be expected and that they have the expected error messages.

Integration tests

Integration tests exist to "check the code runs" for particular test cases. The key integration tests are driven by test/test_demos.py (test_adjoint/test_demos.py in the case of Goalie) in each package, which runs all the scripts in the demos subdirectory.

Formatting tests

See the Formatting section below.

Notes on testing

  • Each package has a <PACKAGE>_REGRESSION_TEST environment variable, which tells the OS a regression test

[Details of the test suite will appear here]

Formatting

[Details on formatting will appear here]

Clone this wiki locally