Skip to content

Checklist for testing and merging a PR

Jump to bottom
Erik Martin-Dorel edited this page Oct 16, 2021 · 21 revisions

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 [email protected]: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 Types

As specified in commit 87bb9b5, the table below summarizes the commit types (lowercase prefixes before a colon) that are recognized by the release-please GHA:

commit type CHANGELOG section title Comments
feat Features Add a new feature (use feat! if it is non-backward compatible)
fix Bug Fixes Patch a bug
revert Reverts Revert commit (include that commit header, SHA1, and motivation)
perf Performance Improvements Change code to improve performance
refactor Code Refactoring Change code without adding a feature nor fixing a bug
deps Dependencies Change external dependencies (e.g., for scopes opam, docker)
build Build System Change the build system (e.g., for scopes dune, make, docker)
test Tests Add missing tests or correct existing tests
ci CI/CD Change the CI/CD configuration
docs Documentation Change documentation only
style Style (hidden by default) Change code without affecting its meaning (white-space, formatting, semi-colons or so)
chore Miscellaneous Chores (hidden by default) Change files unrelated to code, tests, docs, build or ci config

Here is an example commit message header:

fix(grader): Display negative numbers with mandatory parens; Fix #440

TODO: Recap the rules for commit message headers.

See also:

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)

The aim 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.

  • 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 (one 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.

Clone this wiki locally