[Kimchi-devel] About the review process

[Shu Ming]

More and more patches coming into this mailing list and I found some
comments to the patches were not addressed correctly. One scenario was
that reviewer A had concern on the patch, but B and C gave the
"reviewed-by" to th patch, then the maintainer merged the patch into the
upstream without the concern addressed. The concern may be a very good
point to improve the code. In order to eliminate the scenario, I think
we can improve the code review process in this way:

1) To the reviewer, if you have concern on the patch, please give your
opinion in a very clear way like "NACK" opposite to "Reviewed-by" in the
first line of the reply.

2) To the submitter, please address all the concern and have the
reviewer to give a clear opinion.

3) To the maintainer, please don't not merge the patch if the concern is
not addressed.

4) To help the reviewer to understand your code, please post RFC for
your features and have a widely discussion.

One example, see the detail in the bottom,
"Use new control modules in root.py" which was given one concern on the
patch, but the submitter didn't reply to the concern. Then two others
gave the "reviewed-by" to th patch that leads to the patch merged. The
concern on the patch "not sure we can move these code to __all__ or
other attribute of control/__init__.py just from kimchi.control.networks
import *" was not very clear to the patch submitter and the patch
submitter just ignored the comment.

IMO, that is not a clean way to do the review. Here, we do miss the
chance to improve the origin patch.  Finally, another patch "Improve
controller" was post to address the concern which caused code quality
improvement of the original patch delayed and work duplicated.

See the example patch here and the reviewer's comment and concern inline.
---
On 12/24/2013 02:41 AM, Aline Manera wrote:
> --- a/src/kimchi/root.py
> +++ b/src/kimchi/root.py
> @@ -26,13 +26,23 @@ import json
>
>
> from kimchi import auth
> -from kimchi import controller
> from kimchi import template
> from kimchi.config import get_api_schema_file
> from kimchi.control.utils import parse_request
> -
> -
> -class Root(controller.Resource):
> +from kimchi.control.base import Resource
> +from kimchi.control.config import Config
> +from kimchi.control.debugreports import DebugReports
> +from kimchi.control.host import Host
> +from kimchi.control.interfaces import Interfaces
> +from kimchi.control.networks import Networks
> +from kimchi.control.plugins import Plugins
> +from kimchi.control.storagepools import StoragePools
> +from kimchi.control.tasks import Tasks
> +from kimchi.control.templates import Templates
> +from kimchi.control.vms import VMs

not sure we can move these code to __all__ or other attribute of
control/__init__.py just
from kimchi.control.networks import *


+
+
+class Root(Resource):
def __init__(self, model, dev_env):
self._handled_error = ['error_page.400', 'error_page.404',
'error_page.405', 'error_page.406',
@@ -45,17 +55,17 @@ class Root(controller.Resource):
self._cp_config = dict([(key, self.error_development_handler)
for key in self._handled_error])