Personal experiences/anecdotes of the bzr review processes

Martin Pool mbp at canonical.com
Wed Sep 24 02:59:51 BST 2008


On Wed, Sep 24, 2008 at 1:39 AM, Aaron Bentley <aaron at aaronbentley.com> wrote:
>>> I'm not sure what you're saying.  The "Coding Style Guidelines" section
>>> of HACKING is meant to support this.  Do you mean:
>>> - - It's not complete enough?
>>> - - It's not formal enough?
>>> - - It needs to be split out into a separate document?
>>
>> I'm suggesting that unless reviewers can point to a *specific* requirement
>> in that document, then style issues must not hold up approval.  For example,
>> unless that document describes the exact threshold when a function is "too
>> long" and needs to be split, then any observations that a function "may be
>> too long" be declared irrelevant to the technical issue being discussed, and
>> if the reviewer feels strongly enough about it, they should open a launchpad
>> bug to address their concerns and have their style implemented as they can
>> best manage.
>
> That approach seems extreme to me.  I would not be comfortable accepting
>  something with bad style into the codebase just because the HACKING
> file was not prescient enough to predict that particular problem.

I think it would be overly legalistic to take a "whatever is not
specifically prohibited is allowed" approach; we will get into either
spending lots of time trying to define things in the abstract and in
advance.  The point of the hacking document is that when we've agreed
to have a particular standard we record that decision so that it is
not continually revisited, and so that people can know what the
standard is without needing to discover it by getting patches knocked
back.

>>> We're currently discussing reducing the strength of the requirement for
>>> "design clarity", and perhaps style issues should be addressed the same
>>> way.
>>
>> I'm not sure the 2 are related.  "design clarity" is far more important IMO
>> than the specific exception a unittest happens to raise for a feature no-one
>> expects to exist anywhere but Windows in the first place...
>
> I think they are two points on a spectrum, with one end being "beautiful
> and maintainable", and the other end being "ugly and unmaintainable".
> But the reason I brought it up is that we're looking at changing the
> process for one, and we might want to look at changing the other at the
> same time.

There is that spectrum but I am not talking about shifting our target,
rather about getting there in a more efficient and enjoyable way.

For some of these things, it would be ok to merge the patch first and
then merge a followon patch that e.g. reworks an api, moves the tests
to be per-scenario, reworks a function into smaller bits.  I think a
major reason we don't do that at present is that people feel if it's
not done now it won't be done at all.  This leads to the original
patch and it's contributor being commandeered to do that fix.  Why
can't we do it the other way?  It may be that people feel to busy to
do cleanups, it may be the cleanup in the global scope is not so
important, it may be that people are not good at remembering what
cleanups they want to do.  I think it's likely if we could get patches
through faster it would be easier to make cleanups.

-- 
Martin <http://launchpad.net/~mbp/>



More information about the bazaar mailing list