Re: [PATCH] whitespace: fix initial-indent checking

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 16, 2007, 18:16 UTC
Message-ID: <200712161916.44715.jnareb@gmail.com>
In-Reply-To: <20071216162637.GA3934@fieldses.org>

J. Bruce Fields wrote:

Show 23 quoted lines

> On Sun, Dec 16, 2007 at 11:00:55AM +0100, Wincent Colaiuta wrote:
>> El 16/12/2007, a las 10:08, Jakub Narebski escribió:
>>
>>> J. Bruce Fields wrote:
>>>
>>>> This allows catching initial indents like '\t        ' (a tab followed
>>>> by 8 spaces), while previously indent-with-non-tab caught only indents
>>>> that consisted entirely of spaces.
>>>
>>> I prefer to use tabs for indent, but _spaces_ for align. While previous,
>>> less strict version of check catches indent using spaces, this one also
>>> catches _align_ using spaces.
> 
> No, the previous version didn't work for the align-with-spaces case
> either.  Consider, for example,
> 
> struct widget *find_widget_by_color(struct color *color,
>                                     int nth_match, unsigned long flags)
> 
> If following a "indent-with-tabs, align-with-spaces" policy, then the
> initial whitespaace on the second line should be purely spaces
> (otherwise adjusting the tab stops would ruin the alignment).  But
> indent-with-non-tab would flag this as incorrect even before my fix.

Yes, this is (if we want "indent with tab, align with spaces") false positive even with current version of indent-with-non-tab policy, but it is _rare_ false positive.

It is useful because it catches quite common "indent with spaces only", for example if MTA or editor replaces tabs with spaces, or if editor preserves whitespace but it uses spaces for indent.

So for me this version is a good compromise between false positives and catching real indent whitespace errors. The version proposed has IMHO too many false positive, while I guess not catching much more errors in practice.

Show 11 quoted lines

>> I'd say that Jakub's is a fairly common use case (it's used in many places 
>> in the Git codebase too, I think) so it would be a bad thing to change the 
>> behaviour of "indent-with-non-tab".
>>
>> If you also want to check for "align-with-non-tab" then it really should be 
>> a separate, optional class of whitespace error.
> 
> I would agree with you if it were not for the fact that if you're using
> an "indent-with-tabs, align-with-spaces" policy then the only indent
> whitespace problems that you can flag automatically are space-before-tab
> problems; anything else requires knowledge of the language syntax.

Unfortunately quite true (by the way, doesn't new version of "align-with-non-tab" do not work for Python sources?)

Perhaps it should be called "no-8spaces" os something like that: is the width (in columns) of a tab character configurable, by the way?

> So indent-with-non-tab has only ever been useful for projects that
> insist on tabs for all sequences of 8 spaces in the initial whitespace.

IMVVHO the new version of "indent-with-non-tab" (aka "no-8-spaces") is useful _only_ for such project, while old version not only (see comment above).

-- 
Jakub Narebski
Poland