From 96c35e2dd3e823ca8db1da2f88310615872d3a3a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 7 Jun 2022 08:52:08 -0500 Subject: [PATCH] Add more detail on the context/rationale that should be included when contributing (#2432) Follow-up to https://github.com/matrix-org/matrix-js-sdk/pull/1933 Spawning from various recent documents and comments: - https://github.com/vector-im/element-meta/wiki/Review-process - https://github.com/matrix-org/synapse/pull/12846#discussion_r887270734 - https://gitlab.matrix.org/new-vector/internal/-/wikis/Backend/Reviews --- CONTRIBUTING.md | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 158c916a3..7df3845e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,7 +23,18 @@ Things that should go into your PR description: * A changelog entry in the `Notes` section (see below) * References to any bugs fixed by the change (in GitHub's `Fixes` notation) * Describe the why and what is changing in the PR description so it's easy for - onlookers and reviewers to onboard and context switch. + onlookers and reviewers to onboard and context switch. This information is + also helpful when we come back to look at this in 6 months and ask "why did + we do it like that?" we have a chance of finding out. + * Why didn't it work before? Why does it work now? What use cases does it + unlock? + * If you find yourself adding information on how the code works or why you + chose to do it the way you did, make sure this information is instead + written as comments in the code itself. + * Sometimes a PR can change considerably as it is developed. In this case, + the description should be updated to reflect the most recent state of + the PR. (It can be helpful to retain the old content under a suitable + heading, for additional context.) * Include both **before** and **after** screenshots to easily compare and discuss what's changing. * Include a step-by-step testing strategy so that a reviewer can check out the @@ -31,11 +42,6 @@ Things that should go into your PR description: * Add comments to the diff for the reviewer that might help them to understand why the change is necessary or how they might better understand and review it. -Things that should *not* go into your PR description: - * Any information on how the code works or why you chose to do it the way - you did. If this isn't obvious from your code, you haven't written enough - comments. - We rely on information in pull request to populate the information that goes into the changelogs our users see, both for the JS SDK itself and also for some projects based on it. This is picked up from both labels on the pull request and @@ -254,6 +260,12 @@ on Git 2.17+ you can mass signoff using rebase: git rebase --signoff origin/develop ``` +Review expectations +=================== + +See https://github.com/vector-im/element-meta/wiki/Review-process + + Merge Strategy ==============