diff --git a/docs/source/index.rst b/docs/source/index.rst index 7d9631cb9..8345698d3 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -28,7 +28,7 @@ InnerEye-DeepLearning Documentation :maxdepth: 1 :caption: Further reading for contributors - md/pull_requests.md + md/software_design_overview.md md/testing.md md/contributing.md md/deploy_on_aml.md diff --git a/docs/source/md/contributing.md b/docs/source/md/contributing.md index a48beb630..773ebe9dd 100644 --- a/docs/source/md/contributing.md +++ b/docs/source/md/contributing.md @@ -1,20 +1,27 @@ # Contributing -This document describes guidelines for contributing to the InnerEye-DeepLearning repo. +This document contains detailed guidelines for contributing to the InnerEye-DeepLearning repo. + +For the overarching software development process, refer to the [process description](software_development_process.md). ## Submitting pull requests -- DO submit all changes via pull requests (PRs). They will be reviewed and potentially be merged by maintainers after a peer review that includes at least one of the team members. +- DO submit all changes via pull requests (PRs). They will be reviewed and potentially be merged by maintainers after + a peer review that includes at least one of the team members. +- DO NOT mix independent and unrelated changes in one PR. PRs should implement one change, and hence be small. If + in doubt, err on the side of making the PR too small, rather than too big. It reduces the chance for you as the + author to overlook issues. Small PRs are easier to test and easier to review. - DO give PRs short but descriptive names. - DO write a useful but brief description of what the PR is for. - DO refer to any relevant issues and use keywords that automatically close issues when the PR is merged. - DO ensure each commit successfully builds. The entire PR must pass all checks before it will be merged. -- DO address PR feedback in additional commits instead of amending. +- DO link the correct Github issue. +- DO address PR feedback in additional commits instead of amending existing commits. +- DO NOT add any changes that go beyond what is requested by the reviewers. - DO assume that Squash and Merge will be used to merge the commits unless specifically requested otherwise. -- DO NOT submit "work in progress" PRs. A PR should only be submitted when it is considered ready for review. -- DO NOT mix independent and unrelated changes in one PR. -To enable good auto-generated changelogs, we prefix all PR titles with a category string, like "BUG: Out of bounds error when using small images". +To enable good auto-generated changelogs, we prefix all PR titles with a category string, like +"BUG: Out of bounds error when using small images". Those category prefixes must be in upper case, followed by a colon (`:`). Valid categories are - `ENH` for enhancements, new capabilities @@ -36,11 +43,17 @@ your dev box: - DO write unit tests for each new function or class that you add. - DO extend unit tests for existing functions or classes if you change their core behaviour. - DO ensure that your tests are designed in a way that they can pass on the local box, even if they are relying on -specific cloud features. + specific cloud features. +- DO add tests for each bug you fix. When fixing a bug, the suggested workflow is + to first write a unit test that shows the invalid behaviour, and only then start to code up the fix. - DO run all unit tests on your dev box before submitting your changes. The test suite is designed to pass completely -also outside of cloud builds. + also outside of cloud builds. If you are not + a member of the core InnerEye team, note that you may not be able to run some of the unit tests that access the team's + AzureML workspace. A member of the InnerEye team will be happy to assist then. - DO NOT rely only on the test builds in the cloud. Cloud builds trigger AzureML runs on GPU -machines that have a higher CO2 footprint than your dev box. + machines that have a higher CO2 footprint than your dev box. + +More details, in particular on tests that require GPUs, can be found in the [testing documentation](testing.md). ## Creating issues diff --git a/docs/source/md/pull_requests.md b/docs/source/md/pull_requests.md deleted file mode 100644 index 1d6bb0c5a..000000000 --- a/docs/source/md/pull_requests.md +++ /dev/null @@ -1,19 +0,0 @@ -# Suggested Workflow for Pull Requests - -* Pull Requests (PRs) should implement one change, and hence be small. If in doubt, err on the side of making the PR -too small, rather than too big. It reduces the chance for you as the author to overlook issues. Small PRs are easier -to test and easier to review. -* Ensure that the code you add or change is covered by unit tests. When fixing a bug, the suggested workflow is -to first write a unit test that shows the invalid behaviour, and only then start to code up the fix. -* It is best to run all unit tests on your dev box, and check that they pass, before publishing the PR. If you are not -a member of the core InnerEye team, note that you may not be able to run some of the unit tests that access the team's -AzureML workspace. A member of the InnerEye team will be happy to assist then. -* Before publishing your PR, please run PyCharm's code cleanup tools. You can either do that after editing your file, -by pressing Ctrl+Alt+L, or selecting "Reformat code" in the context menu of the file(s) in the project explorer window. -Alternatively, you should tick all of "Reformat code", "Rearrange code", "Optimize imports", "Cleanup", "Scan with mypy" -in the PyCharm version control check-in dialog. -* Only publish your PR for review once you have a build that is passing. You can make use of the "Create as Draft" -feature of GitHub. -* Link the correct Github issue. -* Once you have obtained approval for your change, do not add any changes that go beyond what is requested by the -reviewers. Any additional pushes to the branch will invalidate the approvals you have obtained.