From d930d14735e8454df5b251c2ce2a758167f5762b Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 13 Mar 2020 14:38:04 +0000 Subject: [PATCH 1/9] Add review policy doc This documents various attributes of our overall review policy from code, design, and product perspectives. Fixes https://github.com/vector-im/riot-web/issues/12614 --- docs/review.md | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 docs/review.md diff --git a/docs/review.md b/docs/review.md new file mode 100644 index 0000000000..ebe79588f6 --- /dev/null +++ b/docs/review.md @@ -0,0 +1,67 @@ +# Review Guidelines + +The following summarises review guidelines that we follow for pull requests in +Riot Web and other supporting repos. These are just guidelines (not strict +rules) and may be updated over time. + +## Code Review + +When reviewing code, here are some things we look for and also things we avoid: + +### We review for + +* Correctness +* Performance +* Accessibility +* Security +* Comments and documentation where needed +* Sharing knowledge of different areas among the team +* Ensuring it's something we're comfortable maintaining for the long term + +### We should avoid + +* Style nits that are already handled by the linter +* Dramatically increasing scope + +### Good practices + +* Use empathetic language + * See also [Mindful Communication in Code + Reviews](https://kickstarter.engineering/a-guide-to-mindful-communication-in-code-reviews-48aab5282e5e) + and [How to Do Code Reviews Like a Human](https://mtlynch.io/human-code-reviews-1/) +* Authors should prefer smaller commits for easier reviewing and bisection +* Reviewers should be explicit about required versus optional changes + * Reviews are conversations and the PR author should feel comfortable + discussing and pushing back on changes before making them +* Core team should lead by example through their tone and language +* Take the time to thank and point out good code changes +* Using softer language like "please" and "what do you think?" goes a long way + towards making others feel like colleagues working towards a common goal + +### Workflow + +* Avoid force pushing to a PR after first round of review +* Use merge commits when landing +* PR author merges after review (assuming they have write access) +* Assign issues only when in progress (don’t overly assign things so it’s clear + that anyone on the team can pick up) + +## Design and Product Review + +We want to ensure that all changes to Riot fit with our design and product +vision. We often request review from those teams so they can provide their +perspective. + +In more detail, our usual process for changes that affect the UI or alter user +functionality is: + +* For changes that will go live when merged, always flag Design and Product + teams as appropriate +* For changes guarded by a feature flag, Design and Product review is not + required (though may still be useful) since we can continue tweaking + +As it can be difficult to review design work from looking at just the changed +files in a PR, authors should be prepared for Design and / or Product teams to +request a link to an ad-hoc build of Riot (hosted anywhere) that can be used for +the review. In the future, we [hope to automate +this](https://github.com/vector-im/riot-web/issues/12624) for every PR. From 0b772ca50af5d5ede913eac684b0add0846319e4 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 13 Mar 2020 16:48:03 +0000 Subject: [PATCH 2/9] Tweak issue assigning Co-Authored-By: Travis Ralston --- docs/review.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/review.md b/docs/review.md index ebe79588f6..ae67fa5294 100644 --- a/docs/review.md +++ b/docs/review.md @@ -43,8 +43,7 @@ When reviewing code, here are some things we look for and also things we avoid: * Avoid force pushing to a PR after first round of review * Use merge commits when landing * PR author merges after review (assuming they have write access) -* Assign issues only when in progress (don’t overly assign things so it’s clear - that anyone on the team can pick up) +* Assign issues only when in progress to indicate to others what can be picked up ## Design and Product Review From 1aecc3d7e5bbe3a7a729d949ea42e5a487797c7d Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Mar 2020 10:18:41 +0000 Subject: [PATCH 3/9] Add multi-repo PR linking Co-Authored-By: Travis Ralston --- docs/review.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/review.md b/docs/review.md index ae67fa5294..cdf42b8bf8 100644 --- a/docs/review.md +++ b/docs/review.md @@ -37,6 +37,9 @@ When reviewing code, here are some things we look for and also things we avoid: * Take the time to thank and point out good code changes * Using softer language like "please" and "what do you think?" goes a long way towards making others feel like colleagues working towards a common goal +* Authors should link to other layers of their PR in their PR before requesting + review. Reviewers might be coming from different places and could miss other + required PRs. ### Workflow From bd8b3a9046fd3ab0ac17e441824777ecafcaf259 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Mar 2020 10:21:28 +0000 Subject: [PATCH 4/9] Move multi-repo linking to workflow section --- docs/review.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/review.md b/docs/review.md index cdf42b8bf8..6c5472d2bc 100644 --- a/docs/review.md +++ b/docs/review.md @@ -37,12 +37,12 @@ When reviewing code, here are some things we look for and also things we avoid: * Take the time to thank and point out good code changes * Using softer language like "please" and "what do you think?" goes a long way towards making others feel like colleagues working towards a common goal -* Authors should link to other layers of their PR in their PR before requesting - review. Reviewers might be coming from different places and could miss other - required PRs. ### Workflow +* Authors should link to other layers of their PR in their PR before requesting + review. Reviewers might be coming from different places and could miss other + required PRs. * Avoid force pushing to a PR after first round of review * Use merge commits when landing * PR author merges after review (assuming they have write access) From 3989f4198511c819077d896e363995bcf161531b Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Mar 2020 10:23:27 +0000 Subject: [PATCH 5/9] Clarify merge options --- docs/review.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/review.md b/docs/review.md index 6c5472d2bc..23d8c1c505 100644 --- a/docs/review.md +++ b/docs/review.md @@ -44,7 +44,8 @@ When reviewing code, here are some things we look for and also things we avoid: review. Reviewers might be coming from different places and could miss other required PRs. * Avoid force pushing to a PR after first round of review -* Use merge commits when landing +* Use the GitHub default of merge commits when landing (avoid alternate options + like squash or rebase) * PR author merges after review (assuming they have write access) * Assign issues only when in progress to indicate to others what can be picked up From 34f3671c09d917e387b36a2b6077f8f282b178e7 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Mar 2020 10:59:33 +0000 Subject: [PATCH 6/9] Add notes on review request handling --- docs/review.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/review.md b/docs/review.md index 23d8c1c505..e0e297da6a 100644 --- a/docs/review.md +++ b/docs/review.md @@ -40,6 +40,11 @@ When reviewing code, here are some things we look for and also things we avoid: ### Workflow +* Authors should request review from riot-web team by default (if someone on the + team is clearly the expert in an area, a direct review request to them may be + more appropriate) +* Reviewers should remove the team review request and request review from + themselves when starting a review to avoid double review * Authors should link to other layers of their PR in their PR before requesting review. Reviewers might be coming from different places and could miss other required PRs. From e348546e598cd8e629e861c763894b6f06059ea7 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Mar 2020 10:59:46 +0000 Subject: [PATCH 7/9] Reformat --- docs/review.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/review.md b/docs/review.md index e0e297da6a..ff612e036c 100644 --- a/docs/review.md +++ b/docs/review.md @@ -52,7 +52,8 @@ When reviewing code, here are some things we look for and also things we avoid: * Use the GitHub default of merge commits when landing (avoid alternate options like squash or rebase) * PR author merges after review (assuming they have write access) -* Assign issues only when in progress to indicate to others what can be picked up +* Assign issues only when in progress to indicate to others what can be picked + up ## Design and Product Review From 5808ea2ba12bc7da1eccca6ce8a8a240d2577e0f Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Mar 2020 11:18:29 +0000 Subject: [PATCH 8/9] Add notes about chatting with the team --- README.md | 5 +++++ docs/review.md | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/README.md b/README.md index d5bbcacece..53c86dec35 100644 --- a/README.md +++ b/README.md @@ -223,6 +223,11 @@ Before attempting to develop on Riot you **must** read the [developer guide for `matrix-react-sdk`](https://github.com/matrix-org/matrix-react-sdk), which also defines the design, architecture and style for Riot too. +Before starting work on a feature, it's best to ensure your plan aligns well +with our vision for Riot. Please chat with the team in +[#riot-dev:matrix.org](https://matrix.to/#/#riot-dev:matrix.org) before you +start so we can ensure it's something we'd be willing to merge. + You should also familiarise yourself with the ["Here be Dragons" guide ](https://docs.google.com/document/d/12jYzvkidrp1h7liEuLIe6BMdU0NUjndUYI971O06ooM) to the tame & not-so-tame dragons (gotchas) which exist in the codebase. diff --git a/docs/review.md b/docs/review.md index ff612e036c..576cb2a019 100644 --- a/docs/review.md +++ b/docs/review.md @@ -74,3 +74,8 @@ files in a PR, authors should be prepared for Design and / or Product teams to request a link to an ad-hoc build of Riot (hosted anywhere) that can be used for the review. In the future, we [hope to automate this](https://github.com/vector-im/riot-web/issues/12624) for every PR. + +Before starting work on a feature, it's best to ensure your plan aligns well +with our vision for Riot. Please chat with the team in +[#riot-dev:matrix.org](https://matrix.to/#/#riot-dev:matrix.org) before you +start so we can ensure it's something we'd be willing to merge. From 5b94d8bbd742288513f4bcd5e789b57de82937f4 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 26 Mar 2020 11:28:47 +0000 Subject: [PATCH 9/9] Add line about UI for network activity --- docs/review.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/review.md b/docs/review.md index 576cb2a019..061bea2ed8 100644 --- a/docs/review.md +++ b/docs/review.md @@ -17,6 +17,7 @@ When reviewing code, here are some things we look for and also things we avoid: * Comments and documentation where needed * Sharing knowledge of different areas among the team * Ensuring it's something we're comfortable maintaining for the long term +* Progress indicators and local echo where appropriate with network activity ### We should avoid