diff --git a/README.md b/README.md index d5bbcacecef..53c86dec35e 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 new file mode 100644 index 00000000000..061bea2ed85 --- /dev/null +++ b/docs/review.md @@ -0,0 +1,82 @@ +# 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 +* Progress indicators and local echo where appropriate with network activity + +### 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 + +* 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. +* Avoid force pushing to a PR after first round of review +* 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 + +## 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. + +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.