couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robert Samuel Newson <rnew...@apache.org>
Subject Re: [NOTICE] Updated Bylaws - final readthrough before vote
Date Sat, 19 Jul 2014 08:48:17 GMT
I would like to see a formal change to R-T-C too. I include the useful notes that Cloudant
follows for this model below. Obviously not all this text belongs in the bylaws. I suggest
it, or a variant of it, belongs on our wiki but is referenced in the bylaws, which should
also cover the rules around changing the policy. That is, the bylaws should be able to remain
static while the R-T-C page might evolve over time.

---

We follows a R-T-C protocol for making code changes. Our reviews happen in the form of Pull
Requests on GitHub. Pull Requests (PRs) are a central point of coordination around developing
GitHub and as such we have a well established policy for handling them. This document outlines
the current policies and best practices and each team member is expected to follow them.

Characteristics of a Good Pull Request

	• A logical progression of commits
	• Each commit is a single logical change
		• If a commit message says "Also did XYZ while in this code" that's a strong smell that
it should be two commits
		• Do not mix logic and style changes in a commit
			• Ie, make whitespace changes in their own commit
	• Feel free to commit often and haphazardly when writing code but use tools to reorder
and rewrite commits before issuing a Pull Request
		• Learn how to do interactive rebase
		• git add -p is also useful
	• When responding to Pull Request comments use new commits to address the comments so that
the discussion is kept sane
	• Before merging a Pull Request when all reviewers have given a thumbs up rewrite the history
of the PR and force push it to GitHub
	• Pull Requests are a discussion. You don't have to take everything every reviewer says
as a command but you may be asked for a technical argument on why you don't want to make suggested
changes.

Merging Pull Requests

	• We use a rough equivalent of Apache Voting system
	• Generally a +1 is taken to mean the reviewer understands the change, agrees with the
principle behind it, and is confident that it does what is claimed (and only what is claimed)
	• Generally speaking, if one or two people +1 a change, feel free to merge it. It's rare
to merge without a single +1
	• If someone suggests a change to a PR then that blocks the merge until either the change
is made or there's an agreement on why to not make the change
	• If you suggest a change on a PR it can be helpful to illustrate the level of care you
have for the change
		• Style changes are usually low priority (unless they're egregiously bad violations) so
things like "I disagree with this variable name" shouldn't be merge-stopping issue unless
there's a good reason (ie, don't use a variable "count" to store an average)
		• Algorithmic changes are generally more important
		• Its also perfectly valid to suggest a change with a "Feel free to ignore this" disclaimer.
Gathering reactions to code in the discussion is important even if we don't act on them
	• Use the 'merge' button.
	• Do not merge a branch onto itself.
	• Never move a tag once made public, burn a version number if you have to instead
	• Don't delete the topic branch after merge

——

Proper Commit Messages 

First, read http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

We pride ourselves on having valuable and useful commit messages. This means that we adhere
to a standard and make use of the various sections of a commit message for tooling and other
automated capturing of our development history.

Its important to remember that a Git commit message is one of the most permanent things a
developer will create in their day to day activities. If the commit message is not useful
then it is wasting precious opportunity for conveying historical information about the project.

* Commit messages should reference JIRA tickets
* Bad commit messages are grounds for veto

—

On 19 Jul 2014, at 06:49, Joan Touzet <wohali@apache.org> wrote:

> Jan says:
> 
> ----- Original Message -----
> On CTR (commit-then-review): it’s a leftover from the cvs/svn days, in our git world,
what was equivalent to commit in the old model is merge to master (and release branches) in
ours. And for that we do distinctly follow review-then-commit (http://wiki.apache.org/couchdb/Merge_Procedure)
to ensure to the highest degree that master and release branches are always in a shippable/stable
state. Git, branches and our merge procedure make all this not much of a problem that it was
in the old cvs/svn days. I wonder then, if we should reword the CTR section to reflect our
situation? (Even if committing to, say, a feature branch, that then is reviewed before it
is merged into master or a release branch could be seen as CTR, it is not quite capturing
the same intent).
> --
> 
> I generally agree here, but I'm going to leave this thread open for 24h before changing
the text. The proposal would be to explicitly state that we follow a review-then-commit model
using git branches and pull requests, generally following the GitHub Flow pattern (http://scottchacon.com/2011/08/31/github-flow.html),
and that detail on the process is outside the scope of the Bylaws.
> 
> -Joan


Mime
View raw message