[
https://issues.apache.org/jira/browse/BEAM-6122?focusedWorklogId=169488&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-169488
]
ASF GitHub Bot logged work on BEAM-6122:
----------------------------------------
Author: ASF GitHub Bot
Created on: 26/Nov/18 20:46
Start Date: 26/Nov/18 20:46
Worklog Time Spent: 10m
Work Description: charlesccychen commented on a change in pull request
#7129: [BEAM-6122] Update committer guidelines
URL: https://github.com/apache/beam/pull/7129#discussion_r236419605
##########
File path: website/src/contribute/committer-guide.md
##########
@@ -24,6 +24,24 @@ This guide is for
[committers](https://www.apache.org/foundation/how-it-works.html#committers)
and covers Beam's guidelines for reviewing and merging code.
+## Pull request review objectives
+
+The review process aims for:
+
+* Review iterations should be efficient, timely and of quality (avoid tiny or
out-of-context changes or huge mega-changes)
+* Support efficiency of authoring (don't want to wait on a review for a tiny
bit because GitHub makes it very hard to stack up reviews in sequence / don't
want to have major changes blocked because of difficulty of review)
+* Ease of first-time contribution (encourage to follow [contribution
guildelines](/contribute/#contributing-code)
+ but committer may absorb some extra effort for new contributors)
+* Pull requests and commit messages establish a clear history with purpose and
origin of changes
+* Ability to perform a granular rollback, if necessary (also see
[policies](/contribute/postcommits-policies/))
+
+Granularity of changes:
+
+* We prefer small independent, incremental PRs with descriptive, isolated
commits. Each commit is a single clear change
+* It is OK to keep separate commits for different logical pieces of the code,
if they make reviewing and revisiting code easier
+* Making commits isolated is a good practice, authors should be able to
relatively easily split the PR upon reviewer's request
+* When there are multiple commits in a single PR, every commit that gets
merged should compile and pass tests
Review comment:
I would like to +1 removing this point. It's not feasible to trigger
precommits for each commit, and if we find that tests don't pass for the
overall PR, it is too much of a hassle to suggest that the author should
rewrite the history to satisfy this point (and in practice I think this will be
ignored).
I would also discourage reverting part of a PR and suggest that for
simplicity, the entire PR should be the unit of reversion (say if a PR is found
to break tests); a subsequent PR can roll forward partial changes if necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 169488)
Time Spent: 2h 10m (was: 2h)
> Update committer guidelines
> ---------------------------
>
> Key: BEAM-6122
> URL: https://issues.apache.org/jira/browse/BEAM-6122
> Project: Beam
> Issue Type: Task
> Components: website
> Reporter: Thomas Weise
> Assignee: Thomas Weise
> Priority: Major
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Per discussion in
> [https://lists.apache.org/thread.html/6d922820d6fc352479f88e5c8737f2c8893ddb706a1e578b50d28948@%3Cdev.beam.apache.org%3E]
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
