[webkit-dev] Should we ever change style guidelines?

[Adam Treat]

On Wednesday 09 December 2009 04:02:07 pm Maciej Stachowiak wrote:
> I have thought over your comments about not changing the style
> guidelines at a whim.
>
> I think you make a very good point: the most important thing about the
> style guidelines is that there is one way to do things, and that's
> more important than having the very best way. In particular, anyone
> who is looking to change the style guidelines to best reflect their
> personal preferences is thinking about it the wrong way. And making
> lots of changes could lead to useless code thrash. So, your argument
> almost made me back off on the proposed style rule changes.
>
> However, I don't want to get so set in our ways that we never dare to
> change anything about the style guidelines. One principle that's
> important to the WebKit project is willingness to change things for
> the better, even if there is a short-term cost in terms of disruption.
> We're willing to rename classes and functions, rename files, and break
> internal interfaces if that makes the code better. In the case of the
> style guidelines, how do we balance this way of thinking with the
> desire to avoid thrash and changing on a whim? I think something like
> the following should be our "meta-guidelines" for when to change the
> style guide:

Good point.

> 1) If an issue or a particular case is not addressed in the style rule
> at all, then we should be willing to add a new rule.

I disagree with this.  I think the heavily favored presumption should be 
against adding new rules.  If we discover some supremely annoying 
inconsistency then that is one thing, but it is important to note that *we do 
not need* a rule for every possible different type of style.  I like having a 
consistently styled codebase, but I also like having the flexibility to 
exercise good judgement.  With every new rule to the style guide I fear we 
lose the latter and become ever more pedantic about often trivial issues.

Rather, I'd prefer to think of the style guidelines as just that: guidelines.  
In the end, I believe both patch authors and reviewers should use good common 
sense just like in any other aspect of our codebase.  If for some reason the 
style guidelines are problematic in a particular corner case, then I think we 
should have the flexibility to use our common sense.  I think this already 
occurs, but perhaps it is good to spell out?

I'd like to go back to thinking of the style guidelines as a *guide* for patch 
authors into the common coding style of the community.  What I suspect is 
happening (which concerns me), is the style guidelines being used to work 
around the problems we're encountering scaling the review queue.

If we are to continue on the road of ever more stringent adherence to the 
style <strike>guidelines</strike>rules, then I think we should exact an ever 
heavier burden on adding to or modifying those rules.

> 2) If we failed to fully consider the consequences of a rule when we
> first wrote it down, whether in general or in some particular case,
> then we should consider revising it to address the unanticipated issues.

Sure.

> 3) If we have overwhelmingly strong evidence that a particular change
> would aid readability, even though the issue in question had been
> considered before, then we can consider changing. But there should be
> a heavy presumption in favor of the current rule, and if there is not
> consensus on the change, we should lean towards not changing.

Fine with that.

> 4) If the code actually follows a different pattern than the formal
> rule, and we see no strong advantage to the rule, we should consider
> changing the rule to match the code.

Or perhaps we get rid of the rule?

> I think in the case of case labels, we have a bit of #2 going on. I
> don't think we considered the possibility of blocks inside the case
> label, or at least when we were creating these rules I don't think we
> were aware that there are some cases where using braces is essential,
> so the solution would be to leave them out. The only reason it's every
> truly needed to use braces is if you declare a variable with a type
> that has a constructor or destructor, and I'm pretty sure I at least
> hadn't thought of this when we made the rule.

switch (type) {
case A:
    {
        Foo foo;
        break;
    }
case B:
default:
    return;
}

That's what I've been using and I think this passes the current rule.

> In the case of braces around single-line clauses, I think we also
> didn't fully consider the impact when you have a lengthy if chain and
> a mix of single-line and multi-line statements. So I think there too,
> #2 applies. However, we definitely *did* consider if statements with
> no else, and two-clause if/else statements, and decided we were ok
> with the consequences of the rule.

Well, people still disagree whether this is a readability improvement.  And 
while I am sympathetic that it is, I still consider mine to be a subjective 
opinion and therefore not a good enough reason to change the rule. 

Cheers,
Adam