Code Review Guidelines

The only way to get code into Elastos.ELA.SideChain.ESC is to send a pull request, which needs to be reviewed by someone. This document further explains our expectations around PRs for both authors and reviewers.

Terminology

• The author of a pull request is the entity who wrote the diff and submitted it to GitHub

• The team consists of people with commit rights on the Elastos.ELA.SideChain.ESC repository

• The reviewer is the person assigned to review the differences and must be a team mebers

• The code owner is the person responsible for the subsystem being modified by the PR

The Process

The first decision to make for any PR is whether it's worth including at all. This decision lies primarily with the code owner but may be negotiated with team members.

To make the decision, we must understand what the PR is all about. If there isn't enough descriptive content or the difference is too large, anyone can request an explanation.

We expect that reviewers check the style and functionality of the PR, providing comments to the author using the GitHub review system. Reviewers should follow up with the PR until it is in good shape, then approve it. Approved PRs can be merged by any code owner.

When communicating with authors, be polite and respectful.

Code Style

We expect everyone to use the gofmted code. For contributions of significant sizes, we expect authors to understand and use the guidelines in Effective Go. Authors should avoid common mistakes explained in the Go Code Review Comments page.

Functional Checks

For PRs that fix an issue, reviewers should try to reproduce the issue and verify that the pull request actually fixes it. Authors can help with this by including a unit test that fails without (and passes with) the change.

For PRs adding new features, reviewers should attempt to use the feature and comment on how it feels to use it. For example, if a PR adds a new command line flag, use the program with the flag and comment on whether the flag feels useful.

We expect appropriate unit test coverage. Reviewers should verify that the new code is covered by unit tests.

CI

Code submitted must pass all unit tests and static analysis ("lint") checks. We use Travis CI to test code on Linux, macOS, and AppVeyor to test codes on Microsoft Windows.

For failing CI builds, the issue may not be related to the PR itself. Such failures are usually related to flaky tests. These failures can be ignored (authors don't need to fix unrelated issues), but please file a GH issue so the test gets fixed eventually.

Commit Messages

Commit messages on the master branch should follow the rule below. PR authors are not required to use any particular style because the message can be modified at merge time. Enforcing a commit message style is the responsibility of the person merging the PR.

The commit message style we use is similar to the style used by the Go project:

The first line of the change description is conventionally a one-line summary of the change, prefixed by the primary affected Go package. It should complete the sentence, "This change modifies Elastos.ELA.SideChain.ESC to _____." The rest of the description elaborates and should provide context for the change and explain what it does.

Template:

package/path: change XYZ
 
Longer explanation of the change in the commit. You can use
multiple sentences here. It's usually best to include content
from the PR description in the final commit message.
 
issue notices, e.g. "Fixes #42353".

Special Situations and How To Deal With Them

As a reviewer, you may find yourself in one of the situations below. Here's how to deal with them:

• The author doesn't follow up: Ping them after a while (typically a few days). If there is no further response, close the PR or complete the work yourself.

• The author insists on including refactoring changes alongside bug fixes: We can tolerate small refactorings alongside any change. If you feel lost in the diff, ask the author to submit the refactoring as an independent PR, or at least as an independent commit in the same PR.

• The author keeps rejecting your feedback: Reviewers have the authority to reject any change for technical reasons. If you're unsure, ask the team for a second opinion. You may close the PR if no consensus can be reached.