openstack/zaqar
Code Issues Proposed changes
17e7274349
zaqar/doc/source/reviewer_guide.rst
Eva Balycheva b53ff5d12c Refactoring of docs during Mitaka cycle 
This patch improves Zaqar documentation and fixes currently existing
bugs.

Bugs this patch currently addresses and solutions:

Short names for documentation locations used in this commit message:
GitRepo - https://github.com/openstack/zaqar/
Contributor Docs - http://docs.openstack.org/developer/zaqar/
Wiki - https://wiki.openstack.org/wiki/Zaqar/

1. DRY violation and spreaded information for contributors bug.
The information for Zaqar contributors is spreaded/duplicated across
GitRepo, Contributor Docs and Wiki.
Examples of DRY violation are these three articles:
https://wiki.openstack.org/wiki/Zaqar/Give_Zaqar_a_try,
https://github.com/openstack/zaqar/blob/master/README.rst,
http://docs.openstack.org/developer/zaqar/development-environment.html

Example of spreaded information is: "zaqar/tests/functional/README.rst"
Normally the contributor want to see the information from this file in
"doc/source/running_tests.rst".

Solution: move useful missing information for contributors from Wiki and
GitRepo to Contributor Docs, then replace all contributor information in
Wiki and GitRepo with links to Contributor Docs.

2. Outdated information, missing new information and broken links bug.
Example is "test_suite.rst":
a. It still states that Zaqar test suite lives in two directories -
"tests" and "zaqar/tests", but now it's not true.
b. Doesn't contain information about how test invocation is organized,
what really happens when "tox -e py27" command executes.

Solution: replace outdated information with new, fix broken links if
possible, add useful missing information.

3. Style and formatting bugs.
The reference is http://docs.openstack.org/contributor-guide/.

Many documents in Contributor Docs have wrong line wrapping - some lines
are wrapped too short and some are wrapped too long.
Lines must wrap at 79 characters, exceptions are code and links.
Example is "first_review.rst" which lines are not wrapped at all.

Enumerated lists must be written using "#. " syntax.
Example with wrong enumerated list is "development.environment.rst".

Some inline elements must be styled according to:
http://docs.openstack.org/contributor-guide/rst-conv/inline-markups.html
Example with wrong styling of inline elements is
"development.environment.rst" where many paths and file names are not
marked with `` (double backticks).

By default code inserts are implicitly styled with python syntax.
There are many places in Contributor Docs where console (bash) code is
wrongly styled with python syntax.

Also there are some failed attempts to apply a formatting in Contributor
Docs. For example there is a broken list in "first_review.rst" at line
52.

Solution: fix broken formatting, apply proper style where it is needed.

Some of these bugs fixes closes few bug reports from:
https://etherpad.openstack.org/p/zaqar-mitaka-docs

Change-Id: Id668684248bdee03eb43b537dc2c6bb2a68ed23d
2016-01-07 17:05:09 +03:00

6.1 KiB
Raw Blame History

Reviewer Guide

Overview

Our program follows the usual OpenStack review process, albeit with some important additions (see below). See also: first_review.

Be Professional

The PTL, with the support of the core reviewers, is ultimately responsible for holding contributors accountable for creating a positive, constructive, and productive culture. Inappropriate behavior will not be tolerated. (Why this is important?)

Do This:

  • Act professionally.
  • Treat others as friends and family.
  • Seek first to understand.
  • Be honest, transparent, and constructive.
  • Use clear, concise language.
  • Use prefixes to clarify the tone and intent of your comments.

Don't Do This:

  • Use indecent, profane, or degrading language of any kind.
  • Hold a patch hostage for an ulterior motive, political or otherwise.
  • Abuse the review system to discuss big issues that would be better hashed out on the mailing list, in IRC, or during OpenStack Summit design sessions.
  • Engage in bullying behaviors, including but not limited to:
    • Belittling others' opinions
    • Persistent teasing or sarcasm
    • Insulting, threatening, or yelling at someone
    • Accusing someone of being incompetent
    • Setting someone up to fail
    • Humiliating someone
    • Isolating someone from others
    • Withholding information to gain an advantage
    • Falsely accusing someone of errors
    • Sabotaging someone's work

Reviewing Docs

When possible, enlist the help of a professional technical writer to help review each doc patch. All reviewers should familiarize themselves with OpenStack Documentation Contributor Guide. When reviewing user guide patches, please run them through Maven and proof the resulting docs before giving your +1 or +2.

Reviewing Code

When reviewing code patches, use your best judgment and seek to provide constructive feedback to the author. Compliment them on things they have done well, and highlight possible improvements. Also, dedicate as much time as necessary in order to provide a careful analysis of the code. Don't assume that someone else will catch any issues you yourself miss; in other words, pretend you are the only person reviewing a given patch. Remember, "given enough eyeballs, all bugs are shallow" ceases to be true the moment individual reviewers become complacent.

Some things to check when reviewing code:

  • Patch aligns with project goals, and is ideally associated with a bp or bug.
  • Commit message is formatted appropriately and contains external references as needed.
  • Coding style matches guidelines given in HACKING.rst.
  • Patch is cohesive and not too big to be reviewed in a timely manner (some patches may need to be split to improve cohesion and/or reduce size).
  • Patch does what the commit message promises.
  • Algorithms are implemented correctly, and chosen appropriately.
  • Data schemas follow best practices.
  • Unit and functional tests have been included and/or updated.
  • Code contains no bugs (pay special attention to edge cases that tests may have missed).

Use Prefixes

We encourage the use of prefixes to clarify the tone and intent of your review comments. This is one way we try to mitigate misunderstandings that can lead to bad designs, bad code, and bad blood.

Prefixes
Prefix What the reviewer is saying Blocker?
KUDO You did a nice job here, and I wanted to point that out. Keep up the good work! No
TEST I think you are missing a test for this feature, code branch, specific data input, etc. Yes
BUG I don't think this code does what it was intended to do, or I think there is a general design flaw here that we need to discuss. Yes
SEC This is a serious security vulnerability and we better address it before merging the code. Yes
PERF I have a concern that this won't be fast enough or won't scale. Let's discuss the issue and benchmark alternatives. Yes
DSQ I think there is something critical here that we need to discuss this in IRC or on the mailing list before moving forward. Yes
STYLE This doesn't seem to be consistent with other code and with HACKING.rst Yes
Q I don't understand something. Can you clarify? Yes
DRY This could be modified to reduce duplication of code, data, etc. See also: Wikipedia: Don't repeat yourself Maybe
YAGNI This feature or flexibility probably isn't needed, or isn't worth the added complexity; if it is, we can always add the feature later. See also: Wikipedia: You aren't gonna need it Maybe
NIT This is a nitpick that I can live with if we want to merge without addressing it. No
IMO I'm chiming in with my opinion in response to someone else's comment, or I just wanted to share an observation. Please take what I say with a grain of salt. No
FYI I just wanted to share some useful information. No