From 68dfb2fbf11684f69cd6328871ea2a0922b5d7c8 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 30 Jul 2024 17:03:40 -0300 Subject: [PATCH 1/4] Add merge/squash guidelines to CONTRIBUTING.rst As discussed in https://github.com/pytest-dev/pytest/discussions/12633. --- CONTRIBUTING.rst | 54 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 12e2b18bb52..4970f5f6fc7 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -380,6 +380,56 @@ pull requests from other contributors yourself after having reviewed them. +Merge/squash guidelines +----------------------- + +When a PR is approved and ready to be integrated to the `main` branch, one has the option to *merge* the commits unchanged, or *squash* all the commits into a single commit. + +Here is some guidelines on how to proceeed, based on the state of the PR commit history: + +1. Miscellaneous commits: + + * `Implement X` + * `Fix test_a` + * `Add myself to AUTHORS` + * `!fixup Fix test_a` + * `Update tests/test_integration.py` + * `Update tests/test_integration.py` + + In this case, prefer to use the **Squash** merge strategy: the commit history is a bit messy (not in a derrogatory way, often one just commits changes because they know the changes will eventually be squashed together), so squashing everything into a single commit is best. Prefer to also edit the default GitHub message to add more details and improve on it. + +2. Separate commits related to the same topic: + + * `Implement X` + * `Add myself to AUTHORS` + * `Update CHANGELOG for X` + + In this case, prefer to use the **Squash** merge strategy: while the commit history is not "messy" as in the example above, the individual commits do not bring much value overall, specially when looking at the changes a few months/years down the line. + +3. Separate commits, each with their own topic (refactorings, renames, etc), but still have a larger topic/purpose. + + * `Refactor class X in preparation for feature Y` + * `Remove unused method` + * `Implement feature Y` + + In this case, prefer to use the **Merge** strategy: each commit is valuable on its own, even if they serve a common topic overall. Looking at the history later, it is useful to have the removal of the unused method separately on its own commit, along with more information (such as how it became unused in the first place). + +4. Separate commits, each with their own topic, but without a larger topic/purpose other than improve the code base (using more modern techniques, improve typing, removing clutter, etc). + + * `Improve internal names in X` + * `Add type annotations to Y` + * `Remove unnecessary dict access` + * `Remove unreachable code due to EOL Python` + + In this case, prefer to use the **Merge** strategy: each commit is valuable on its own, and the information on each is valuable in the long term. + + +As mentioned, those are overall guidelines, not rules cast in stone. This topic was discussed in [#12633](https://github.com/pytest-dev/pytest/discussions/12633). + + +*Backports* should always be **squashed**, as they preserve the original PR author. + + Backporting bug fixes for the next patch release ------------------------------------------------ @@ -438,6 +488,8 @@ above? All the above are not rules, but merely some guidelines/suggestions on what we should expect about backports. +Backports should be **squashed** (rather than **merged**), as doing so preserves the original PR author correctly. + Handling stale issues/PRs ------------------------- @@ -485,7 +537,7 @@ When closing a Pull Request, it needs to be acknowledging the time, effort, and -Closing Issues +Closing issues -------------- When a pull request is submitted to fix an issue, add text like ``closes #XYZW`` to the PR description and/or commits (where ``XYZW`` is the issue number). See the `GitHub docs `_ for more information. From 2befa248f762b7c214d585b467ebbaf41b8492e8 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 31 Jul 2024 16:36:24 -0300 Subject: [PATCH 2/4] Update CONTRIBUTING.rst --- CONTRIBUTING.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 4970f5f6fc7..13f465c15e7 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -427,7 +427,7 @@ Here is some guidelines on how to proceeed, based on the state of the PR commit As mentioned, those are overall guidelines, not rules cast in stone. This topic was discussed in [#12633](https://github.com/pytest-dev/pytest/discussions/12633). -*Backports* should always be **squashed**, as they preserve the original PR author. +*Backports* (as those created automatically from a `backport` label) should always be **squashed**, as they preserve the original PR author. Backporting bug fixes for the next patch release From cfd435653e224c1131cb4a9c9ceae95c572975b8 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 31 Jul 2024 20:38:06 -0300 Subject: [PATCH 3/4] Review suggestions --- CONTRIBUTING.rst | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 13f465c15e7..e9edcef4a0c 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -383,51 +383,52 @@ them. Merge/squash guidelines ----------------------- -When a PR is approved and ready to be integrated to the `main` branch, one has the option to *merge* the commits unchanged, or *squash* all the commits into a single commit. +When a PR is approved and ready to be integrated to the ``main`` branch, one has the option to *merge* the commits unchanged, or *squash* all the commits into a single commit. -Here is some guidelines on how to proceeed, based on the state of the PR commit history: +Here are some guidelines on how to proceed, based on examples of a single PR commit history: 1. Miscellaneous commits: - * `Implement X` - * `Fix test_a` - * `Add myself to AUTHORS` - * `!fixup Fix test_a` - * `Update tests/test_integration.py` - * `Update tests/test_integration.py` + * ``Implement X`` + * ``Fix test_a`` + * ``Add myself to AUTHORS`` + * ``!fixup Fix test_a`` + * ``Update tests/test_integration.py`` + * ``Merge origin/main into PR branch`` + * ``Update tests/test_integration.py`` - In this case, prefer to use the **Squash** merge strategy: the commit history is a bit messy (not in a derrogatory way, often one just commits changes because they know the changes will eventually be squashed together), so squashing everything into a single commit is best. Prefer to also edit the default GitHub message to add more details and improve on it. + In this case, prefer to use the **Squash** merge strategy: the commit history is a bit messy (not in a derogatory way, often one just commits changes because they know the changes will eventually be squashed together), so squashing everything into a single commit is best. You must clean up the commit message, making sure it contains useful details. 2. Separate commits related to the same topic: - * `Implement X` - * `Add myself to AUTHORS` - * `Update CHANGELOG for X` + * ``Implement X`` + * ``Add myself to AUTHORS`` + * ``Update CHANGELOG for X`` In this case, prefer to use the **Squash** merge strategy: while the commit history is not "messy" as in the example above, the individual commits do not bring much value overall, specially when looking at the changes a few months/years down the line. 3. Separate commits, each with their own topic (refactorings, renames, etc), but still have a larger topic/purpose. - * `Refactor class X in preparation for feature Y` - * `Remove unused method` - * `Implement feature Y` + * ``Refactor class X in preparation for feature Y`` + * ``Remove unused method`` + * ``Implement feature Y`` In this case, prefer to use the **Merge** strategy: each commit is valuable on its own, even if they serve a common topic overall. Looking at the history later, it is useful to have the removal of the unused method separately on its own commit, along with more information (such as how it became unused in the first place). 4. Separate commits, each with their own topic, but without a larger topic/purpose other than improve the code base (using more modern techniques, improve typing, removing clutter, etc). - * `Improve internal names in X` - * `Add type annotations to Y` - * `Remove unnecessary dict access` - * `Remove unreachable code due to EOL Python` + * ``Improve internal names in X`` + * ``Add type annotations to Y`` + * ``Remove unnecessary dict access`` + * ``Remove unreachable code due to EOL Python`` In this case, prefer to use the **Merge** strategy: each commit is valuable on its own, and the information on each is valuable in the long term. -As mentioned, those are overall guidelines, not rules cast in stone. This topic was discussed in [#12633](https://github.com/pytest-dev/pytest/discussions/12633). +As mentioned, those are overall guidelines, not rules cast in stone. This topic was discussed in `#12633 `_. -*Backports* (as those created automatically from a `backport` label) should always be **squashed**, as they preserve the original PR author. +*Backport PRs* (as those created automatically from a ``backport`` label) should always be **squashed**, as they preserve the original PR author. Backporting bug fixes for the next patch release From dac2eb12348afde33a88bd775b995d1022c42af1 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 31 Jul 2024 21:57:10 -0300 Subject: [PATCH 4/4] Fix fixup message example --- CONTRIBUTING.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index e9edcef4a0c..6e96fd24c40 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -392,7 +392,7 @@ Here are some guidelines on how to proceed, based on examples of a single PR com * ``Implement X`` * ``Fix test_a`` * ``Add myself to AUTHORS`` - * ``!fixup Fix test_a`` + * ``fixup! Fix test_a`` * ``Update tests/test_integration.py`` * ``Merge origin/main into PR branch`` * ``Update tests/test_integration.py``