Differential D25007 Diff 23 src/docs/flavor/writing_reviewable_code.diviner

Changeset View

Standalone View

src/docs/flavor/writing_reviewable_code.diviner

@title Writing Reviewable Code		@title Writing Reviewable Code
@group review		@group review

Project recommendations on how to structure changes.		Project recommendations on how to structure changes.

This document is purely advisory. Phabricator works with a variety of revision		This document is purely advisory. Phorge works with a variety of revision
control strategies, and diverging from the recommendations in this document		control strategies, and diverging from the recommendations in this document
will not impact your ability to use it for code review and source management.		will not impact your ability to use it for code review and source management.

= Overview =		= Overview =

This document describes a strategy for structuring changes used successfully at		This document describes a strategy for structuring changes used successfully at
Facebook and in Phabricator. In essence:		Facebook and in Phorge. In essence:

- Each commit should be as small as possible, but no smaller.		- Each commit should be as small as possible, but no smaller.
- The smallest a commit can be is a single cohesive idea: don't make commits		- The smallest a commit can be is a single cohesive idea: don't make commits
so small that they are meaningless on their own.		so small that they are meaningless on their own.
- There should be a one-to-one mapping between ideas and commits:		- There should be a one-to-one mapping between ideas and commits:
each commit should build one idea, and each idea should be implemented by		each commit should build one idea, and each idea should be implemented by
one commit.		one commit.
- Turn large commits into small commits by dividing large problems into		- Turn large commits into small commits by dividing large problems into
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines
The minimum size of a change should be a complete implementation of the simplest		The minimum size of a change should be a complete implementation of the simplest
subproblem which works on its own and expresses an entire idea, not just part		subproblem which works on its own and expresses an entire idea, not just part
of an idea. You could mechanically split a 1,000-line change into ten 100-line		of an idea. You could mechanically split a 1,000-line change into ten 100-line
changes by choosing lines at random, but none of the individual changes would		changes by choosing lines at random, but none of the individual changes would
make any sense and you would increase the collective complexity. The real goal		make any sense and you would increase the collective complexity. The real goal
is for each change to have minimal complexity, line size is just a proxy that is		is for each change to have minimal complexity, line size is just a proxy that is
often well-correlated with complexity.		often well-correlated with complexity.

We generally follow these practices in Phabricator. The median change size for		We generally follow these practices in Phorge. The median change size for
Phabricator is 35 lines.		Phorge is 35 lines.

= Write Sensible Commit Messages =		= Write Sensible Commit Messages =

There are lots of resources for this on the internet. All of them say pretty		There are lots of resources for this on the internet. All of them say pretty
much the same thing; this one does too.		much the same thing; this one does too.

The single most important thing is: **commit messages should explain //why// you		The single most important thing is: **commit messages should explain //why// you
are making the change**.		are making the change**.
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Lines	really all that important in commit messages include:
- If/where text is wrapped.		- If/where text is wrapped.
- Maximum length of the title.		- Maximum length of the title.
- Whether there should be a period or not in the title.		- Whether there should be a period or not in the title.
- Use of voice/tense, e.g. "fix"/"add" vs "fixes"/"adds".		- Use of voice/tense, e.g. "fix"/"add" vs "fixes"/"adds".
- Other sorts of pedantry not related to getting the context and		- Other sorts of pedantry not related to getting the context and
reasons //why// a change is happening into the commit message.		reasons //why// a change is happening into the commit message.
- Although maybe the spelling and grammar shouldn't be egregiously bad?		- Although maybe the spelling and grammar shouldn't be egregiously bad?

Phabricator does not have guidelines for this stuff. You can obviously set		Phorge does not have guidelines for this stuff. You can obviously set
guidelines at your organization if you prefer, but getting the //why// into the		guidelines at your organization if you prefer, but getting the //why// into the
message is the most important part.		message is the most important part.

= Next Steps =		= Next Steps =

Continue by:		Continue by:

- reading recommendations on structuring revision control with		- reading recommendations on structuring revision control with
@{article:Recommendations on Revision Control}; or		@{article:Recommendations on Revision Control}; or
- reading recommendations on structuring branches with		- reading recommendations on structuring branches with
@{article:Recommendations on Branching}.		@{article:Recommendations on Branching}.