Skip to content

Checklist for testing and merging a PR

Jump to bottom
Erik Martin-Dorel edited this page Oct 12, 2023 · 21 revisions

Old remark

  • A manual commit bumping the "version" string in *.opam.locked used to be necessary (similarly to 1f73141),
    within the Release-PR branch, or just after the previous release in master branch.
  • This is now unneeded since release-please v13.5.0.

Guidelines to fetch and test a Pull Request (say, PR #200) locally

  • install the git-pr and git-prw extra Git commands;
  • cd in the learn-ocaml repository (in the sequel, we assume the remote origin is bound to URL git@github.com:ocaml-sf/learn-ocaml.git);
  • run git pr 200 origin -f to force-fetch a read-only branch for PR #200,
    or git prw 200 origin -f to get a read-and-write branch, i.e., git push will land in the PR-author's work branch.

Checklist before merging a PR

  • The CI checks must be green.
  • Non-trivial PRs should have been tested locally (see the Guidelines above).
  • New user-facing features should be documented within the PR, in directory docs/.
  • The commits messages should follow the Conventional Commits guidelines.
    This is necessary for properly generating the Release-Notes using release-please.

Conventional Commits

See the related documentation directly in CONTRIBUTING.md.

Merging a PR

There are 3 PR merging strategies in GitHub (Merge, Merge/Squash, Merge/Rebase).

But for merging PRs, we should rather use Merge or Merge/Squash:

  • Merge is always a good, standard choice;

  • Merge/Squash is especially fine if we don't want to keep the commit history of the PR branch or if the PR already contains only one commit;

  • whereas Merge/Rebase has two drawbacks:

    1. there is no mention anywhere of the PR number in the generated commits messages;
    2. and the resulting commits are not GPG-signed (cf. the missing Verified badge on those commits).

    Actually, Merge/Rebase might be handy in the specific case of "nested PR merging" (namely: we would Merge/Rebase an intermediate PR into a feature branch (for which the intermediate PR number would be useless), then create a subsequent PR, and finally Merge it from the feature branch intoto the master branch itself).

Conventional Commits and (Merge vs. Merge/Squash)

One of the aims of this paragraph is to refine/relax the constraint above "The commits messages should follow the Conventional Commits guidelines."

  • First, if Merge/Squash looks a sensible choice
    (e.g. if the PR contains only one commit, or only focuses on one topic for which keeping the commits apart does not sound necessary),
    then the conventional commit constraint is lifted for the PR contributor,
    given the Maintainer can easily refine the squashed-commit message, selecting an appropriate commit type.

  • Note by the way that if two pending PRs contain common commits
    (e.g. if the second PR has been created on top of the first one),
    then merging the first PR with Merge/Squash would always create a conflict with the second PR,
    while a regular Merge would not have this drawback.

  • Next, if Merge looks a better choice
    (e.g. if the PR contains several independent commits that have to be kept for clarity),

    • then the Merge commit message does not matter (the Maintainer could just select the default one);
    • but each individual commit of the PR should be relevant:
      for instance, partially-implemented feature commits, followed by fixups commits (even with a prefix fix:) are not acceptable, given the changelog will ultimately display only one commit for the feat: Feature title, so this one should be complete, unless of course if it can be split in several sensible atomic commits.

  • Finally, note that for simplicity, Release PRs should rather be integrated using Merge/Squash. See e.g. PR #442.

A few useful labels

Clone this wiki locally