Apply prettier formatting
This commit is contained in:
Michael Weimann
parent
a32f12c8f3
commit
7921a6cbf8
104 changed files with 12169 additions and 11047 deletions
|
|
@ -10,53 +10,53 @@ When reviewing code, here are some things we look for and also things we avoid:
|
|||
|
||||
### We review for
|
||||
|
||||
* Correctness
|
||||
* Performance
|
||||
* Accessibility
|
||||
* Security
|
||||
* Quality via automated and manual testing
|
||||
* 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
|
||||
- Correctness
|
||||
- Performance
|
||||
- Accessibility
|
||||
- Security
|
||||
- Quality via automated and manual testing
|
||||
- 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
|
||||
- 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
|
||||
* Reviewers are encouraged to ask for tests where they believe it is reasonable
|
||||
* 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
|
||||
- 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
|
||||
- Reviewers are encouraged to ask for tests where they believe it is reasonable
|
||||
- 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 the element-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
|
||||
* If there are multiple related PRs authors should reference each of the PRs in
|
||||
the others before requesting review. Reviewers might start reviewing from
|
||||
different places and could miss other required PRs.
|
||||
* Avoid force pushing to a PR after the 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
|
||||
- Authors should request review from the element-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
|
||||
- If there are multiple related PRs authors should reference each of the PRs in
|
||||
the others before requesting review. Reviewers might start reviewing from
|
||||
different places and could miss other required PRs.
|
||||
- Avoid force pushing to a PR after the 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
|
||||
|
||||
## Code Quality
|
||||
|
||||
|
|
@ -64,10 +64,10 @@ In the past, we have occasionally written different kinds of tests for
|
|||
Element and the SDKs, but it hasn't been a consistent focus. Going forward, we'd
|
||||
like to change that.
|
||||
|
||||
* For new features, code reviewers will expect some form of automated testing to
|
||||
be included by default
|
||||
* For bug fixes, regression tests are of course great to have, but we don't want
|
||||
to block fixes on this, so we won't require them at this time
|
||||
- For new features, code reviewers will expect some form of automated testing to
|
||||
be included by default
|
||||
- For bug fixes, regression tests are of course great to have, but we don't want
|
||||
to block fixes on this, so we won't require them at this time
|
||||
|
||||
The above policy is not a strict rule, but instead it's meant to be a
|
||||
conversation between the author and reviewer. As an author, try to think about
|
||||
|
|
@ -104,10 +104,10 @@ 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
|
||||
- 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, a [preview site](./pr-previews.md) that includes your changes
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue