mirror of https://github.com/zulip/zulip.git
docs: Write down some high-level thoughts on doing code reviews.
This started as a PSA in the form of a series of chat messages in `#general` on chat.zulip.org; putting them here, with some editing, to make their value more durable. Also rearrange this doc slightly so that it's not specific to the server codebase, except in a few explicit spots. The bit that's for authors should probably be somewhere else. I think there isn't right now a great natural spot for it -- probably the top of docs/git-guide, some parts of docs/version-control, and that paragraph here should all turn into a top-level "guide to submitting code to Zulip" doc, which would link to the rest of docs/git-guide and to some other resources. Leaving that for another day.
This commit is contained in:
parent
5c9f02924f
commit
8e82bf6532
|
@ -1,10 +1,69 @@
|
|||
# Reviewing Zulip server code
|
||||
# Reviewing Zulip code
|
||||
|
||||
This document is a brief discussion of what we look for when reviewing
|
||||
contributions to Zulip. It's meant partially for developers who want
|
||||
to get their code merged faster, and partially for developers who have
|
||||
made successful pull requests already and would like to start
|
||||
participating in code review.
|
||||
Code review is a key part of how Zulip does development! If you've
|
||||
been contributing to Zulip's code, we'd love for you to do reviews.
|
||||
This is a guide to how. (With some thoughts for writing code too.)
|
||||
|
||||
## Principles of code review
|
||||
|
||||
### Anyone can review
|
||||
|
||||
Anyone can do a code review -- you don't have to have a ton of
|
||||
experience, and you don't have to have the power to ultimately merge
|
||||
the PR. If you
|
||||
|
||||
* read the code, see if you understand what the change is
|
||||
doing and why, and ask questions if you don't; or
|
||||
|
||||
* fetch the code (for Zulip server code,
|
||||
[tools/fetch-rebase-pull-request][git tool] is super handy), play around
|
||||
with it in your dev environment, and say what you think about how
|
||||
the feature works
|
||||
|
||||
those are really helpful contributions.
|
||||
|
||||
### Please do reviews
|
||||
|
||||
Doing code reviews is an important part of making the project go.
|
||||
It's also an important skill to develop for participating in
|
||||
open-source projects and working in the industry in general. If
|
||||
you're contributing to Zulip and have been working in our code for a
|
||||
little while, we would love for some of your time contributing to come
|
||||
in the form of doing code reviews!
|
||||
|
||||
For students participating in Google Summer of Code or a similar
|
||||
program, we expect you to spend a chunk of your time each week (after
|
||||
the first couple of weeks as you're getting going) doing code reviews.
|
||||
|
||||
### Fast replies are key
|
||||
|
||||
For the author of a PR, getting feedback quickly is really important
|
||||
for making progress quickly and staying productive. That means that
|
||||
if you get @-mentioned on a PR with a request for you to review it,
|
||||
it helps the author a lot if you reply promptly.
|
||||
|
||||
A reply doesn't even have to be a full review; if a PR is big or if
|
||||
you're pressed for time, then just getting some kind of reply in
|
||||
quickly -- initial thoughts, feedback on the general direction, or
|
||||
just saying you're busy and when you'll have time to look harder -- is
|
||||
still really valuable for the author and for anyone else who might
|
||||
review the PR.
|
||||
|
||||
People in the Zulip project live and work in many timezones, and code
|
||||
reviewers also need focused chunks of time to write code and do other
|
||||
things, so an immediate reply isn't always possible. But a good
|
||||
benchmark is to try to always reply **within one workday**, at least
|
||||
with a short initial reply, if you're working regularly on Zulip. And
|
||||
sooner is better.
|
||||
|
||||
### Protocol for authors
|
||||
|
||||
When you send a PR, try to think of a good person to review it --
|
||||
outside of the handful of people who do a ton of reviews -- and
|
||||
`@`-mention them with something like "`@person`, would you review
|
||||
this?". Good choices include
|
||||
* someone based in your timezone or a nearby timezone
|
||||
* people working on similar things, or in a loosely related area
|
||||
|
||||
## Things to look for
|
||||
|
||||
|
@ -43,20 +102,9 @@ participating in code review.
|
|||
user input, and potential bugs that are likely for the type of
|
||||
change being made. Tests that exclude whole classes of potential
|
||||
bugs are preferred when possible (e.g., the common test suite
|
||||
`test_bugdown.py` between the [frontend and backend Markdown
|
||||
processors](markdown.html) or the `GetEventsTest` test for buggy
|
||||
race condition handling).
|
||||
|
||||
Backend: we are trying to maintain ~100% test coverage on the
|
||||
backend, so backend changes should have negative tests for the
|
||||
various error conditions. Frontend: If the feature involves frontend
|
||||
changes, there should be frontend tests. See the [test
|
||||
writing][test-writing] documentation for more details.
|
||||
|
||||
* *mypy annotations.* New functions should be annotated using [mypy]
|
||||
and existing annotations should be updated. Use of `Any`, `ignore`,
|
||||
and unparameterized containser should be limited to cases where a
|
||||
more precise type cannot be specified.
|
||||
`test_bugdown.py` between the Zulip server's [frontend and backend
|
||||
Markdown processors](markdown.html), or the `GetEventsTest` test for
|
||||
buggy race condition handling).
|
||||
|
||||
* *Translation.* Make sure that the strings are marked for
|
||||
[translation].
|
||||
|
@ -104,15 +152,34 @@ participating in code review.
|
|||
control][commit-messages] documentation for details on what we look
|
||||
for.
|
||||
|
||||
### Zulip server
|
||||
|
||||
Some points specific to the Zulip server codebase:
|
||||
|
||||
* *Testing -- Backend.* We are trying to maintain ~100% test coverage
|
||||
on the backend, so backend changes should have negative tests for
|
||||
the various error conditions.
|
||||
|
||||
* *Testing -- Frontend.* If the feature involves frontend changes,
|
||||
there should be frontend tests. See the [test
|
||||
writing][test-writing] documentation for more details.
|
||||
|
||||
* *mypy annotations.* New functions should be annotated using [mypy]
|
||||
and existing annotations should be updated. Use of `Any`, `ignore`,
|
||||
and unparameterized containser should be limited to cases where a
|
||||
more precise type cannot be specified.
|
||||
|
||||
## Tooling
|
||||
|
||||
To make it easier to review pull requests, use our [git tool]
|
||||
To make it easier to review pull requests, if you're working in the
|
||||
Zulip server codebase, use our [git tool]
|
||||
`tools/fetch-rebase-pull-request` to check out a pull request locally
|
||||
and rebase it against master. If a pull request just needs a little
|
||||
fixing to make it mergeable, feel free to do that in a new commit,
|
||||
then push your branch to GitHub and mention the branch in a comment on
|
||||
the pull request. That'll save the maintainer time and get the PR
|
||||
merged quicker.
|
||||
and rebase it against master.
|
||||
|
||||
If a pull request just needs a little fixing to make it mergeable,
|
||||
feel free to do that in a new commit, then push your branch to GitHub
|
||||
and mention the branch in a comment on the pull request. That'll save
|
||||
the maintainer time and get the PR merged quicker.
|
||||
|
||||
## Additional Resources
|
||||
|
||||
|
|
Loading…
Reference in New Issue