From 2f801d3d4a1b08129ff50bb32fdb0db8cf565cf5 Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:07:03 +0100 Subject: [PATCH 1/8] Reformat branch sections in contribution guidelines --- CONTRIBUTING.md | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b04b8afc..92664ad6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,19 +39,23 @@ The following describes the workflow for contributions. We loosely follow a mixture of [GitHubFlow](https://docs.github.com/en/get-started/quickstart/github-flow) and [GitFlow](https://nvie.com/posts/a-successful-git-branching-model/): -`main` - - Stable branch - - Merges come only from `develop` or a hotfix branch (i.e., when something needs to be fixed in "production") +#### `main` -`develop` - - Unstable development branch - - Last "stable" is tagged: tag before attempting to break something +- Stable branch +- Merges come only from `develop` or a hotfix branch (i.e., when something needs to be fixed in "production") -`feature/` (including issue id when exists: `feature/62-improve-broken-thing`/`feature/42-add-new-thing`) - - Branch from last tag on `develop` +#### `develop` -`hotfix/` (including issue id when exists: `hotfix/62-fix-broken-thing-in-release`) - - Branch from `main` +- Unstable development branch +- Last "stable" is tagged: tag before attempting to break something + +#### `feature/` (including issue id when exists: `feature/62-improve-broken-thing`/`feature/42-add-new-thing`) + +- Branch from last tag on `develop` + +#### `hotfix/` (including issue id when exists: `hotfix/62-fix-broken-thing-in-release`) + +- Branch from `main` ### Pull requests From 4951f4325170a66f512c9a99a7c9c78a319a4a5d Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:11:30 +0100 Subject: [PATCH 2/8] Fix wording and subsection format --- CONTRIBUTING.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 92664ad6..46c5a4bd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ The following describes the workflow for contributions. ### Branching -We loosely follow a mixture of [GitHubFlow](https://docs.github.com/en/get-started/quickstart/github-flow) and [GitFlow](https://nvie.com/posts/a-successful-git-branching-model/): +We loosely follow a mixture of [GitHubFlow](https://docs.github.com/en/get-started/quickstart/github-flow) and [GitFlow](https://nvie.com/posts/a-successful-git-branching-model/) with the following branches. #### `main` @@ -49,12 +49,14 @@ We loosely follow a mixture of [GitHubFlow](https://docs.github.com/en/get-start - Unstable development branch - Last "stable" is tagged: tag before attempting to break something -#### `feature/` (including issue id when exists: `feature/62-improve-broken-thing`/`feature/42-add-new-thing`) +#### `feature/` -- Branch from last tag on `develop` +- Naming convention: include an issue id if one exists, e.g., `feature/62-improve-broken-thing` or `feature/42-add-new-thing` +- Branch from updated `develop` -#### `hotfix/` (including issue id when exists: `hotfix/62-fix-broken-thing-in-release`) +#### `hotfix/` +- Naming convention: include an issue id if one exists, e.g., `hotfix/62-fix-broken-thing-in-release` - Branch from `main` ### Pull requests From 14099b67fcd856e8417eedc38efde196c5674251 Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:29:47 +0100 Subject: [PATCH 3/8] Fix section anchor generation by MyST - Markdown header anchors aren't generated automatically anymore, see https://myst-parser.readthedocs.io/en/latest/develop/_changelog.html#markdown-link-resolution-improvements. - Set heading anchors depth to 4 to re-enable behaviour --- docs/source/conf.py | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/source/conf.py b/docs/source/conf.py index 7f63ec70..1c7a0103 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -76,6 +76,7 @@ 'tasklist', 'deflist', ] +myst_heading_anchors = 4 # Sphinx API docs configuration, see https://sphinx-autoapi.readthedocs.io/en/latest/reference/config.html autoapi_type = "python" From 812eda4f6d3b0489c65df7c9f08b333b9ecb3780 Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:34:02 +0100 Subject: [PATCH 4/8] Fix wording in contribution guidelines --- CONTRIBUTING.md | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 46c5a4bd..41ac6de1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,35 +41,36 @@ We loosely follow a mixture of [GitHubFlow](https://docs.github.com/en/get-start #### `main` -- Stable branch -- Merges come only from `develop` or a hotfix branch (i.e., when something needs to be fixed in "production") +- This is the stable branch. +- Merges into `main` come only from `develop` or a hotfix branch (i.e., when something needs to be fixed in "production"). #### `develop` -- Unstable development branch -- Last "stable" is tagged: tag before attempting to break something +- This is the unstable development branch. +- Before you attempt to break something or add experimental changes, `git tag` the current state. #### `feature/` -- Naming convention: include an issue id if one exists, e.g., `feature/62-improve-broken-thing` or `feature/42-add-new-thing` -- Branch from updated `develop` +- Naming convention: include an issue id if one exists, e.g., `feature/62-improve-broken-thing` or `feature/42-add-new-thing`. +- Branch from last tag on `develop`. #### `hotfix/` -- Naming convention: include an issue id if one exists, e.g., `hotfix/62-fix-broken-thing-in-release` -- Branch from `main` +- Naming convention: include an issue id if one exists, e.g., `hotfix/62-fix-broken-thing-in-release`. +- Branch from `main`. -### Pull requests +### Pull requests (PRs) -Project members may create pull requests from co-located branches, while external contributors need to following -a [forking pattern](https://docs.github.com/en/get-started/quickstart/fork-a-repo). +Project members may create pull requests from branches in the main repository, while external contributors need to follow +a [forking pattern](https://docs.github.com/en/get-started/quickstart/fork-a-repo). In both cases, please follow these rules: -- As soon as you have made 1 commit in a feature branch, put up a *draft* pull request -- Keep pull requests small :skull: -- :warning: No pre-emptive reviews on PR drafts, **unless** the PR author @-mentions with this specific request -- When you think you're done, mark PR ready for review, see below (Merge Process) -### Merge Process (into `develop`) +- As soon as you have made 1 commit in a feature branch, put up a *draft* pull request. +- Keep pull requests small. +- ⚠️ Do not review *draft* pull requests, unless the PR author @-mentions you with this specific request. +- When you think you're done, mark the PR ready for review to start the [merge process](#merging-changes-into-develop). + +### Merging changes into `develop` - Create PR from `feature/...` against `develop` (PR author) - Describe work in initial comment (PR author) From f762c3d08f28c3e593607663582b7c7d78dd9506 Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:51:53 +0100 Subject: [PATCH 5/8] Fix contribution guideline contents for readability and tone --- CONTRIBUTING.md | 110 ++++++++++++++++++++++++------------------------ 1 file changed, 56 insertions(+), 54 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 41ac6de1..2d904cf2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,68 +72,70 @@ a [forking pattern](https://docs.github.com/en/get-started/quickstart/fork-a-rep ### Merging changes into `develop` -- Create PR from `feature/...` against `develop` (PR author) -- Describe work in initial comment (PR author) +- *Create draft PR:* The **contributor** [creates a draft pull request (PR)](#pull-requests-prs) from `feature/...` against `develop` (and becomes the **PR author**). +- *Describe changes:* The **PR author** describes the changes in the PR in the initial comment: - Reference any related issues (use, e.g., `Fixes #n` or `- Related: #n`) - - What does the new code do? - - Optional: What should reviewers look at specifically - - Information on how to review: - - E.g. + - Describe what new code does + - Optional: Describe what reviewers should look at specifically + - Include information on how to review: + - E.g.: ```bash - pip install ./ - pytest test/ + poetry install + poetry run pytest test/ ``` -- Request review (PR author) - - Eligible reviewers: - - Python code: @led02, @sdruskat, @jkelling - - Documentation: +- *Request review:* The **PR author** requests one or more reviews. + - Eligible reviewers are: + - For Python code: @led02, @sdruskat, @jkelling + - For Documentation: - Workflow: @all - Project: @all -- Review (at least 1 reviewer) - - Follow instructions in PR +- *Review:* At least 1 **reviewer** reviews the changes: + - Follow the instructions in the PR - Review thoroughly beyond instructions - - Comments / change suggestions inline in files - - Submit review: - - **CASE 1:** Non-blocking change requests (typos, documentation wording, etc.) -> Document and Accept - - **CASE 2:** Blocking change requests (something doesn't work, bad quality code, docs not understandable): - - Ideally, fix things yourself in the branch -> Request Changes (or do them on your own) - - **CASE 3:** "Just a comment"s, pointers to potential future changes -> Document and Accept - - **CASE 4:** :warning: If you want a second pair of eyes on the PR, use "Comment" to finish review, request - another reviewer and @-mention in comment. - - Optional: If you find something blocking after initial review, add review with "Request changes" outcome -- Act on review (PR author) + - Add comments or change suggestions inline in the respective file using GitHub's * changed* tab + - Submit the review with the correct review outcome: + - **CASE 1:** Non-blocking change requests (typos, documentation wording, etc.) -> *Document and Accept* + - **CASE 2:** Blocking change requests (something doesn't work, bad quality code, docs are not understandable): + - Ideally, fix things yourself in the branch -> *Request Changes* (or do them on your own) + - **CASE 3:** "Just a comment"s, pointers to potential future changes -> *Document and Accept* + - **CASE 4:** ⚠️ If you want a second pair of eyes on the PR, use *Comment* to finish the review, then request + another reviewer and @-mention them in a comment on the PR. + - Optional: If you find something blocking after your initial review, add another review with *Request changes* outcome. +- *Act on review:* The **PR author** acts on the review - React to comments - - Fix issues + - Fix issues (including non-blocking issues) - Discuss options -- Then: - - CASE 1: PR author to fix non-blocking issues and merge - - CASE 2: PR author to re-request review from original reviewer(s) - - CASE 3: PR author to react to comments and merge - - CASE 4: (PR author's reaction depends on outcome of second review) -- Re-review: - - See Review above -- Any maintainer - - Close PR if PR is not suitable for merge, and no further changes to improve it come from the PR author, - after having communicated sensible requests with a deadline for further work in the PR comments. - - Merge PR and delete remote branch if at least half of the invited reviewers have approved the PR, and no changes + - Then: + - **CASE 1:** The **PR author** merges the PR. + - **CASE 2:** The **PR author** re-requests a review from the original reviewer(s). + - **CASE 3:** The **PR author** reacts to any comments. If all comments are resolved, the **PR author** merges the PR. + - **CASE 4:** The correct next step depends on the outcome of the second review. +- *Re-review:* + - *See Review* above +- **Any maintainer** can at any time: + - Close a PR if the PR is not suitable for merging, and no further changes to improve it come from the PR author. + ⚠️ Only do this after after having communicated sensible requests with a deadline for further work in the PR comments. + - Merge a PR and delete the remote branch if at least half of the invited reviewers have approved the PR, and no changes have been requested after review. This implements lazy consensus to avoid bottlenecks, where a PR has been approved by some reviewers but cannot be closed due to missing reviews. -### Release/stabilization process - -- Create release branch `release/v` from `develop` -- Check if everything looks good - - Audit source (using linters and stuff) - - Ensure test coverage of at least 72.3% - - Check if documentation aligns with code (also run tutorial to check completeness) - - Check if metadata is correct -- Put up PR from release branch against `main` -- Request review (workflow as above) -- Merge into `main` -- Tag `main` HEAD as `v` -- Push `main` -- Push tag -- Merge `main` into `develop` -- Delete release branch -- :bulb: If something goes wrong in the release branch, you can always delete, fix things in a feature branch, merge - into `develop` following workflow above, and start anew +### Stabilizing the codebase and making releases + +⚠️ The following steps can only be taken by maintainers. + +- Create a release branch `release/v` from `develop`. +- Check if everything looks good: + - Audit the source code (using linters and other tooling). + - Ensure test coverage is at least 65%, and that all tests pass. + - Check if the documentation aligns with the code (also run tutorial to check completeness). + - Check if the metadata is correct in all relevant places. +- Put up a PR from the release branch against `main`. +- Request a review (using the same workflow as above). +- Merge the PR into `main`. +- Tag `main`'s `HEAD` as `v`. +- Push `main`. +- Push tag. +- Merge `main` into `develop`. +- Delete the release branch. +- 💡 If something goes wrong in the release branch, you can always delete it, fix things in a feature branch, merge + into `develop` following the workflow above, and start anew. From 1ea3bbe51ca55fa98dd7d159ba43c6317d720963 Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:53:38 +0100 Subject: [PATCH 6/8] Fix rule what maintainers can do when --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2d904cf2..b0a49964 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -112,7 +112,7 @@ a [forking pattern](https://docs.github.com/en/get-started/quickstart/fork-a-rep - **CASE 4:** The correct next step depends on the outcome of the second review. - *Re-review:* - *See Review* above -- **Any maintainer** can at any time: +- **Any maintainer** can: - Close a PR if the PR is not suitable for merging, and no further changes to improve it come from the PR author. ⚠️ Only do this after after having communicated sensible requests with a deadline for further work in the PR comments. - Merge a PR and delete the remote branch if at least half of the invited reviewers have approved the PR, and no changes From 357d47884cb03a2bab9b996e7543abfc971105d9 Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:54:57 +0100 Subject: [PATCH 7/8] Use enumeration for release process --- CONTRIBUTING.md | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b0a49964..d019553c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -123,19 +123,19 @@ a [forking pattern](https://docs.github.com/en/get-started/quickstart/fork-a-rep ⚠️ The following steps can only be taken by maintainers. -- Create a release branch `release/v` from `develop`. -- Check if everything looks good: - - Audit the source code (using linters and other tooling). - - Ensure test coverage is at least 65%, and that all tests pass. - - Check if the documentation aligns with the code (also run tutorial to check completeness). - - Check if the metadata is correct in all relevant places. -- Put up a PR from the release branch against `main`. -- Request a review (using the same workflow as above). -- Merge the PR into `main`. -- Tag `main`'s `HEAD` as `v`. -- Push `main`. -- Push tag. -- Merge `main` into `develop`. -- Delete the release branch. -- 💡 If something goes wrong in the release branch, you can always delete it, fix things in a feature branch, merge +1. Create a release branch `release/v` from `develop`. +1. Check if everything looks good: + 1. Audit the source code (using linters and other tooling). + 1. Ensure test coverage is at least 65%, and that all tests pass. + 1. Check if the documentation aligns with the code (also run tutorial to check completeness). + 1. Check if the metadata is correct in all relevant places. +1. Put up a PR from the release branch against `main`. +1. Request a review (using the same workflow as above). +1. Merge the PR into `main`. +1. Tag `main`'s `HEAD` as `v`. +1. Push `main`. +1. Push tag. +1. Merge `main` into `develop`. +1. Delete the release branch. +1. 💡 If something goes wrong in the release branch, you can always delete it, fix things in a feature branch, merge into `develop` following the workflow above, and start anew. From 82eb580193c6d83b2d400cfc17253b9ef659142d Mon Sep 17 00:00:00 2001 From: Stephan Druskat Date: Wed, 18 Jan 2023 13:56:21 +0100 Subject: [PATCH 8/8] Swap contribution guidelines and tutorial in TOC --- docs/source/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/index.md b/docs/source/index.md index ffa3c081..b0c9efe8 100644 --- a/docs/source/index.md +++ b/docs/source/index.md @@ -38,8 +38,8 @@ automatic submission to publication repositories. ```{toctree} :maxdepth: 1 :caption: Developers -Tutorial: Get started w/ development dev/contribute +Tutorial: Get started w/ development dev/data_model adr/index api/index