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
This commit is contained in:
+18
-6
@@ -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
|
||||
==============
|
||||
|
||||
|
||||
Reference in New Issue
Block a user