About the review process
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])
On 01/27/2014 04:25 PM, Shu Ming wrote:
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. first line: NACK, need to improve
hope, "NACK" does not make the submitter unhappy.
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.
and detail commit message? also for some big feature: Tells how use and verify this feature ?
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])
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 01/27/2014 06:25 AM, Shu Ming wrote:
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.
Hi Ming, Thanks for your note. I agree with most of the points you exposed here. I just want to make one thing clearer: will be rare the cases where everyone will agree with the patch implementation. Because that we have some patches with "Reviewed-by" and "NACK". In that cases, if the maintainer does not agree with the "NACK" the patch will be merged. Otherwise, the maintainer will block it.
--- 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])
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (3)
-
Aline Manera -
Sheldon -
Shu Ming