Skip to content

Commit

Permalink
update contributing guide (#1361)
Browse files Browse the repository at this point in the history
  • Loading branch information
@phritz
phritz authored Nov 26, 2018
1 parent 1529ec0 commit bb85e8a
Show file tree
Hide file tree
Showing 3 changed files with 203 additions and 178 deletions.
246 changes: 116 additions & 130 deletions CONTRIBUTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,13 @@
:+1::tada: First off, thanks for taking the time to contribute! :tada::+1:

The following is a set of guidelines for contributing to the Filecoin
Project. Feel free to propose changes to this document in a pull
request. These guidelines can and should change as the project changes
phases.

Also: these guidelines should not replace common sense. The golden rule
is __if something feels wrong, stop and surface the issue.__

Filecoin, including go-filecoin and all related modules, follows the
Project. Feel free to propose changes, as these guidelines are a living
document. Filecoin, including go-filecoin and all related modules, follows the
[Filecoin Code of Conduct](CODE_OF_CONDUCT.md).

### Table Of Contents
* [Enable Progress](#enable-progress)
* [Developer Do's and Don'ts](#developer-dos-and-donts)
* [Code Reviews](#code-reviews)
* [Project Management](#project-management)
* [Conventions and Style](#conventions-and-style)
* [Error Handling](#error-handling)
* [The Spec](#the-spec)
Expand All @@ -27,89 +20,105 @@ Filecoin, including go-filecoin and all related modules, follows the
* [Pull Requests](#pull-requests)
* [Gotchas](#gotchas)

## Enable Progress

A primary directive for every dev is to _enable other devs to make
progress_. We increase our leverage and velocity by enabling our
teammates. Be an enabler -- find ways to help the person make progress.
Don't be a gate-keeper -- someone who introduces un-actionable
or unnecessary obstacles to progress. Prioritize code reviews and
answering questions over your own work to a reasonable extent.
Prioritize progress over perfection when possible.

On the flip side: make progress. If you need help, ask for it. Ask
for immediate feedback in slack, a short video meeting, or a group
code walk-through if it would be helpful.
## Developer Do's and Don'ts

In order to keep the project moving efficiently we ask that contributors
adhere to the following guidance:

* **DO enable progress**: As a contributor your _#1 priority_ should
be enabling the progress of other contributors. When we enable our
collaborators we increase our leverage and velocity. Don't be a gate-keeper,
someone who introduces unactionable or unnecessary obstacles to progress.
* Corollary: *make progress*. Don't get stuck. Ask for help.
* Corollary: *prioritize code reviews and requests for
feedback*. Progress is measured by working code that delivers
values to users, please help people get it merged.

* **DO NOT create technical debt**: We have passed the point in the project
where "do the simplest thing possible that works" is a good strategy. We need
to be completing features so that they are "done done", and not deferring
an ever-increasing amount of work into the future. This is the only way
to make the project converge. Half-completed features give us a false sense
of progress and kludgy features are even worse: we end up doing the work twice,
once as a hack and then a second time right. Build it right from the start.
* Example: you add a cache for something. To prevent the cache
from becoming a DOS vector it requires the addition of a tricky
replacement policy. You are tempted to defer that implementation
because there is a lot else to do. "Do not create technical debt"
means that you should implement the replacement policy along with the cache,
and not defer that work into the future (thereby creating
technical debt).
* Example: to add your feature you can do a hacky thing that you know is
not a good long term solution and will be painful for whomever has to touch
the code next. Or you could do the right thing which is going to take you
longer. "Do not create technical debt" means do the right thing.

* **DO capture design intent on paper, ahead of implementation, and
subject it to constructive feedback**: explicitly identify the
problem you are solving, the requirements and constraints the
solution must meet, the proposal, its rationale, and any
alternatives we should not pursue. Essentially, write a
[designdoc](designdocs.md). The length and detail should be
commensurate with the size, risk, and importance of the thing in
question. Note there is a [similar process for protocol-level
changes](https://github.com/filecoin-project/specs/blob/master/process.md).

* **DO NOT add dependencies on `Node` or add more implementation to `Node`**: The
`Node` has become a god object and a dependency of convenience. Abstractions
should not take a `Node` as a dependency: they should take the narrower
set of things they actually need. Building blocks should not go on `Node`,
they should go into separate, smaller abstractions that depend on the
narrow set of things they actually need. [More detail in this issue](https://github.com/filecoin-project/go-filecoin/issues/1223#issuecomment-433764709).

* **DO embrace asynchronous workflows**: To the extent possible, drive work
asynchronously through issues. Accept as both a strength and constraint
of the project that sometimes the best person to do your code review is 12
time zones away, and it might take you several days to get a PR in because of
it.

## Code Reviews

With "prioritize progress" as a primary directive we can derive some
With "enable progress" as a primary directive we can derive some
corollaries for code reviews:

* Unless a reviewee asks for it, **avoid lengthy design discussions
in PR reviews**. Design discussions in PRs shouldn't consume a lot
of time or be open-ended. If it seems like something is going to
take more than a few quick iterations or would require sweeping
changes, prefer to merge and defer the design discussion to a follow
up issue (and then follow up). Recognize that when this happens it
might point to a process failure where design should've been
discussed earlier. If that's the case go fix that problem as well.

Exercise judgement about when not to defer design discussion. For
example if deferring would ultimately require lots of painful
refactoring or undoing lots of work, consider not deferring.

* Limit scope of comments to the story itself: avoid feature creep.
As above, prefer to defer the addition of new features to followup
work.

* Get comfortable with "good enough" and recognize that "good
enough" is context-dependent. For example "good enough" is different
for v0 block explorer than for the consensus algorithm. Ask
yourself if, given the context, whether the details you're about to
ask someone to spend time changing will materially affect the
success of the project. If not, consider holding off.
* **Avoid lengthy design discussions in PR reviews** unless the
reviewee asks for it. Prefer to merge and spin out an issue for
discussion if the conversation snowballs, and then follow up on the
process failure that led to needing a big design discussion in a PR.

* Pinging reviewers to look at a review is encouraged.

As for approvals:
* 2 approvals for all PRs (except typos, etc) with at least one of {@whyrusleeping, @dignifiedquire, @phritz}
* 3/3 for far-reaching/foundational or protocol level changes

In order to minimize the time wasted in code review and handoff we have
the following protocol:
* prefer to go to in-person/voice if the PR is open >= 3 days
* pinging reviewers to look at a PR considered useful and encouraged
* comments should be actionable and clearly state what is requested
* by default code review comments are advisory: reviewee
* 2 approvals for all PRs, with at least one of {@whyrusleeping, @dignifiedquire, @phritz}
* 3/3 of the above for far-reaching/foundational or protocol level changes

### Code review protocol:
* Comments should be actionable and clearly state what is requested.
* By default, code review comments are advisory: reviewee
should consider them but doesn't _have_ to respond or
address them
* a comment that says "BLOCKING" must be addressed
and responded to. A reviewer has to decide how
to deliver a blocking comment: via "Request Changes" (merge blocking) or via
"Add Comments" or "Approve" (not merge blocking):
address them.
* A comment that starts with "BLOCKING" must be addressed and responded
to. A reviewer has to decide how to deliver a blocking comment: via
"Request Changes" (merge blocking) or via "Add Comments" or "Approve"
(not merge blocking):
* If a reviewer makes a blocking comment while blocking merge
("Request Changes") they are signaling that they want to have another
look after the chages are made. Don't merge until they approve. Example:
the whole design of an abstraction is wrong and reviewer wants to see it
reworked.
* If a reviewer makes a blocking comment but does not block
merging ("Add Comments" or "Approve") then the reviewee can
merge if the issue is addressed. Example: you have an off by one
error. It's mandatory to fix but doesn't necessarily require
another look from the reviewer.
merge if the issue is addressed. Example: reviewer points out
an off by one error in a blocking comment, as they Approve the
PR. You have to fix the error but the reviewer trusts you to do
that and you can merge without further review once you have.
* Approval means approval even in the face of minor changes.
github should be configured to allow merging with earlier
approval even after rebasing/minor changes.

**Do not just leave comments in a code review.** Comments should be
blocking or come with an approval unless you are still looking things
over or you're asking for clarification. It's ok/encouraged to ask
for explanations. The thing we want to avoid is *unnecessarily*
requiring mutiple round trips from someone whose next availability
might be 12 hours away.

## Project Management

Details in [dev-process.md](dev-process.md).
Comments should be blocking or come with an approval unless you are
still looking things over or you're asking for clarification. It's
ok/encouraged to ask for explanations, in the PR or in IRC.

## Conventions and Style

Expand All @@ -133,7 +142,11 @@ There are always exceptions but generally:
* `Warning`: noteworthy but not completely unexpected
* `Error`: a truly unexpected condition that should not happen in Real Life and that a dev should go look at
* Protocol messages are nouns (eg, `DealQuery`, `DealResponse`) and their handlers are verbs (eg, `QueryDeal`)
* Do not put implementation inline in command functions. Command implementation should be minimal, calling out functionality that exists elsewhere (eg on the node). Commands implementation is an important API which gets muddled when implementation happens inline in command functions.
* Do not put implementation inline in command functions. Command
implementation should be minimal, calling out functionality that
exists elsewhere (but NOT on the node). Commands implementation is
an important API which gets muddled when implementation happens
inline in command functions.

We use the following import ordering.
```
Expand Down Expand Up @@ -173,26 +186,24 @@ incorrect view of the world.

* If an error is likely a function of an input, discard the input.
* If an error could be transient, attempt to continue making progress.
* If an error appears to be permanent, panic.
* If we find we have inconsistent internal state, panic.
* If an error appears to be permanent or we have inconsistent internal state, error out to the top level
and exit cleanly if possible.
* **DO NOT INTENTIONALLY CRASH THE NODE**. Don't panic() if you can exit cleanly. panic()ing stops the node
from doing useful work that might be unrelated to the error and does not give subsystems an opportunity to
clean up, thus potentially creating additional problems.

We should log an ERROR only in truly unexpected conditions
that should not happen and that a dev should go look at.

## The Spec

The spec must be in sync with the code. Updates to the spec should
happen before the code is merged into master. Significant changes to
the spec requires discussion and buy-in from {@nicola, @whyrusleeping,
@dignifiedquire}.
The Filecoin Specification (https://github.com/filecoin-project/specs) must be in sync with the code.

At present (Q1'18) we give ourselves some leeway with spec/code update
lag: it's ok for it to be out of sync for a short period of time while
exploring the right thing to do in the code. The important thing at
this early stage is "the spec is updated" rather than "the spec is
updated in precise lockstep with the code". That will be the policy at
some point in the future.
TODO(Q4'18): we need an updated process here.

## Platform Considerations

Prefer to derive patterns from more established, closely related
It often makes sense to derive patterns from more established, closely related
platforms than to derive them from first principles. For example for
things like configuration, commands, persistence, and the like we draw
heavily on patterns in IPFS. Similarly we draw on patterns in Ethereum
Expand All @@ -201,7 +212,7 @@ related platforms.

## Testing
* All code must be unit tested.
* We prefer to test the output/contracts, not the individual lines of code (which we expect to change significantly during early work).
* Prefer to test the output/contracts.
* Daemon tests (integration tests that run a node and send it commands):
* Daemon tests are not a substitute for unit tests: the foo command implementation should be unit tested in the `foo_test.go` file
* Daemon tests should test integration, not comprehensive functionality
Expand All @@ -226,53 +237,28 @@ Then, use pprof to view the dump:
go tool pprof /tmp/profile.dump
```

## Test Ranger

From time to time we will integrate tests that fail sporadically. Allowing these tests to remain in the codebase is not acceptable.
The Test Ranger will take responsibility for all tests merged into master that fail in continuous integration. While the
Test Ranger is responsible for keeping flaky tests out of the code base, all Filecoin developers are expected to give
priority to helping the ranger over current tasks when asked. The test ranger will also works toward better testing strategy and
serve as a respository of testing knowledge.

## What is the bar for inclusion in master?

Presently (Q1'18) the minimum bar is:
* Must be unittested (80% target, some slippage ok); if PR is a command, it must be command tested
* Code review (see above)
* Must lint (`go run ./build/*.go lint`)
* CI must be green
* Must match a subset of the spec, unless when considering changes to the spec -- see details above
* Functionality should be _somehow_ user-visible (eg, via command line interface)
* Should be bite-sized

Present (Q1'18) NON-requirements:
* Doesn’t have to do all or even much of anything it will ultimately do
* Doesn't have to be fast

Likely future requirements:
* Integration tested
* Respects @jbenet’s most important API requirements
Presently (Q4'18) the minimum bar is:
* Unit tests.
* Tests must not be flaky.
* Must pass CI.
* Code review (see above).
* Lint (`go run ./build/*.go lint`).
* Must match a subset of the spec.
* Documentation is up to date.

## Pull Requests

#### Workflow
* When creating a pull request:
* Move the corresponding issue into `Review/QA`
* Assign reviewers to the pull request
* Place pull request into `This Sprint`.
* Once something is merged to `master` close the issue and move it to `Closed`. (Closing an issue moves it to `Closed` automatically)
* In PRs and commit messages, reference the issue(s) you are addressing with [keywords](https://help.github.com/articles/closing-issues-using-keywords/) like `Closes #123`.

#### Size
* PRs should modify no more than 400 lines or 8 files
* If unavoidable to make a larger PR, break changes into clearly scoped commits. It is ok if individual commits do not build when making the PR. Only squashed commits are required to build.

#### Style
* PRs should modify no more than 400 lines or 8 files. If unavoidable
to make a larger PR, break changes into clearly scoped commits. It
is ok if individual commits do not build when making the PR. Only
squashed commits are required to build.
* Always squash commits.
* Anyone is free to merge but slight preference to let the creator
merge because they have most context. Creators, if you really care
about the commit message, squash ahead of time and provide a nice
commit message because someone might merge for you.
* Anyone is free to merge but preference to let the creator merge
because they have most context. Creators, if you really care about
the commit message, squash ahead of time and provide a nice commit
message because someone might merge for you.

## Where should I start?
* Take a look at issues labelled [E-good-first-issue](https://github.com/filecoin-project/go-filecoin/issues?q=is%3Aopen+is%3Aissue+label%3AE-good-first-issue) or [E-help-wanted](https://github.com/filecoin-project/go-filecoin/issues?q=is%3Aopen+is%3Aissue+label%3AE-helped-wanted).
Expand Down
Loading

0 comments on commit bb85e8a

Please sign in to comment.