Jump to: navigation, search

Difference between revisions of "Documentation/ReviewGuidelines"

(The Waiting Game)
(Review Scoring and Approvals)
Line 66: Line 66:
  
 
Note: If you find an issue with a patch that already has a +2 score from another core reviewer, consider commenting on the issue and scoring the patch with a 0 rather than scoring it with a -1.
 
Note: If you find an issue with a patch that already has a +2 score from another core reviewer, consider commenting on the issue and scoring the patch with a 0 rather than scoring it with a -1.
 +
 +
====Set WorkInProgress (WIP) during review ====
 +
The WIP tag tells anyone looking at a change that more updates are still needed.  Both the change's owner and any core reviewer can set the WIP statusː
 +
* A change owner can set this tag on their own review to mark that additional changes are still being made, and to avoid unnecessary reviews while that happens. 
 +
* A core reviewer can set the WIP tag to acknowledge that a contributor will definitely need to do more work on a change rather than merely expressing an opinion on its readiness.
 +
 +
This can be a great convenience to fellow reviewers. It allows the core reviewer to politely send the message that the
 +
change needs additional work while simultaneously removing it from the list of ready changes until that happens.
 +
 +
To add the WIP tag:
 +
# Click a patch set.
 +
# Click the Review button.
 +
# Choose the '-1 Work In Progress' from the Workflow options, enter comments (optional).
 +
# Click Publish Comments.
 +
 +
This sets a -1 and informs everyone that the patch is WorkInProgress.
 +
 +
=== Abandoning Patches ===
 +
Patches that will not go in, should be abandoned to clean the review list. Both the patch owner and cores can abandon a patch - and also resurrect it again.
  
 
=== Considerations for Documentation Aligned with Release Cycles ===
 
=== Considerations for Documentation Aligned with Release Cycles ===

Revision as of 05:31, 27 July 2014

Goal

Provide guidelines to improve the quality and speed of the documentation review process.

Critique Categories

Objective

  1. Commit message
    1. Content
    2. Conventions
      1. Tags
  2. Patch
    1. Content
    2. Conventions
    3. Grammar
    4. Style/Phrasing/Wording
    5. Spelling
    6. Documentation checklist

Subjective

  1. Commit message
    1. Grammar
    2. Spelling
    3. Style/Phrasing/Wording
  2. Patch
    1. Grammar
    2. Style/Phrasing/Wording
    3. Other suggestions

Scope

  1. Try to keep reviews limited to the contents of the bug, contents of the commit message, and changes made by the patch. In other words, if the patch solves the stated problem, but there are other improvements it could lead to, approve the patch and file a subsequent bug, rather than -1'ing the patch with a comment about improvements.

Consistency

  1. If you find an issue, do your best to mark all instances of it.
    1. If the author uploads a patch correcting your objective issue and you find another instance that you didn't mark, comment on it and score with a -1. Preferably, upload a patch to fix it.
    2. If the author uploads a patch correcting your subjective issue and you find another instance that you didn't mark, comment on it and score with a 0.
    3. If the author uploads a patch correcting your objective and/or subjective issue and you find another objective issue, comment on it and score with a -1. Preferably, upload a patch to fix it.
    4. If the author uploads a patch correcting your objective and/or subjective issue and you find another subjective issue, comment on it and score with a 0.
  2. If you find an issue that could affect other portions of a book, provide appropriate comments, score the patch with a -1, and consider mentioning your issue on the mailing list or in a meeting.
    1. Example: A new service uses "key = value" in the configuration file and all other services use "key=value" in their configuration files. Both methods work, but the book should maintain consistency.

Tagging Additional Reviewers

  1. In some cases, you should tag one or more people with interest in or experience with the content of your patch to review it.
    1. How long should an author wait for reviews by these people?

The Waiting Game

  1. After the first review with a -1 or 0score, an author should update the patch and does not need to wait for any time.
  2. Core reviewers will in general review a patch within a few days. If no review happens, feel free to ask on the #openstack-docs IRC channel.

Review Scoring and Approvals

  1. Scores available to contributors
    1. -1, 0, +1
  2. Scores available to core reviewers
    1. -2, -1, 0, +1, +2
  3. Approvals
    1. A core reviewer can approve a patch with +4 points, typically after it obtains two +2 scores from other core reviewers.
    2. A core reviewer can +2 score a patch with a +2 score from another core reviewer and approve it.
    3. A core reviewer can +2 score a patch with two +1 from other reviewers and approve it.

Note: If you find an issue with a patch that already has a +2 score from another core reviewer, consider commenting on the issue and scoring the patch with a 0 rather than scoring it with a -1.

Set WorkInProgress (WIP) during review

The WIP tag tells anyone looking at a change that more updates are still needed. Both the change's owner and any core reviewer can set the WIP statusː

  • A change owner can set this tag on their own review to mark that additional changes are still being made, and to avoid unnecessary reviews while that happens.
  • A core reviewer can set the WIP tag to acknowledge that a contributor will definitely need to do more work on a change rather than merely expressing an opinion on its readiness.

This can be a great convenience to fellow reviewers. It allows the core reviewer to politely send the message that the change needs additional work while simultaneously removing it from the list of ready changes until that happens.

To add the WIP tag:

  1. Click a patch set.
  2. Click the Review button.
  3. Choose the '-1 Work In Progress' from the Workflow options, enter comments (optional).
  4. Click Publish Comments.

This sets a -1 and informs everyone that the patch is WorkInProgress.

Abandoning Patches

Patches that will not go in, should be abandoned to clean the review list. Both the patch owner and cores can abandon a patch - and also resurrect it again.

Considerations for Documentation Aligned with Release Cycles

  1. Beginning with milestone releases, shift focus to objective issues, especially with new services and existing services with significant changes. Only patches with significant subjective issues should receive a -1 score. Otherwise, comment on subjective issues and score with a 0.
  2. Beginning with release candidates, focus almost entirely on content issues. Only comment on subjective issues if the patch should receive a -1 score for objective issues.