http://qs321.pair.com?node_id=799033

mandog has asked for the wisdom of the Perl Monks concerning the following question:

There has been quite a lot of good discussion on code review, but I haven't seen anything specific on keeping the perfect from being the enemy of the good. Is the checklist below worth the effort of following it?

PURPOSE: establish policies/process for merging
         branches into main line of development.

Before a branch gets merged or rejected:

	Two members of mvhub-dev vote 'approve' 

	zero members vote 'disapprove'
OR:
	Fearless Leader makes an arbitrary and capricious decision.

At each stage of the process, all question should be answered 'yes' 

Answering 'no' or "I don't know", means it isn't ok enough to proceed. 

Before proposing a merge:

	Do all the tests pass?

	Does this merge make trunk better in any way?

	Does this merge NOT make trunk worse in ANY way?

	Does merge have only one purpose?

	Do I accept responsibility for this merge?

Before approving a merge:
Can I say 'yes' to all pre-merge proposal questions above? Do I understand all changes? Code is ok with: app-mvhub/doc/policy/code_format.txt? Code/design is as clean as reasonably possible? Does the merge request meet it's stated goal? Before abstaining: If two other people vote 'approve', no harm will be done? Before voting 're-submit' or 'disapprove' on a merge: I am NOT disapproving because related improvements remain to be made. EXAMPLES: Bob submits: + for ( my $i = 1 ; $i < $param_no + 1 ; $i++ ) { + $param = $test_href->{ 'param' . $i }; + do_something_with($param); + } Betty votes 're-submit' because there is a cleaner way to write the code: foreach my $key (keys %$test_href){ do_something_with($$test_href{$key}); } Sue submits: a merge request with a new .t file to check all SQL keywords are capitalized. Bob votes: 'approve' He thinks that the test should also check if each key word is onl a line by it's own. However, he votes 'approve' because that Ann votes: 'resubmit' 'INSERT' is a key word and Sue's .t file doesn't catch that. Joe submits a merge request that: 1) updates the checklist for moving to production 2) applies approved perltidrc to a .pm file Swetha votes: 'disapprove' Joe should have submitted two separate merge requests.