[dojo-contributors] A new build tool for style guide violation checking

Peter E Higgins dante at dojotoolkit.org
Mon May 11 10:05:55 EDT 2009


Mike Wilcox wrote:
> It sounds like you are mixing two different validations - code  
> integrity (braces in if statements)  and formating integrity (proper  
> tabbing and minimum whitespace).
>   
Maybe I'm confused, but I'm talking about our "style guidelines".

if (foo) { return; }

should be:

if(foo){ return; }

and explicitly NOT:

if(foo) return;

> I think I joined Dojo slightly after that jslint discussion. Since I  
> use jslint to validate my code and check for errors, and my IDE Komodo  
> uses it, I'd like to see it used in Dojo. But a subset as you say,  
> because there are a few things in it I don't agree with. It certainly  
> would clean up the missing semi colons!
>
>   
Missing semicolons aren't the problem. Mixed whitespace/tabspace is.
lint will help you find errors and warnings,
the goal here is to ensure the code we ship meets dojo's guidelines and
follows it consistently.
> But I'd like to point out that whatever JS validation we use, it needs  
> to not be too strict. Currently all my code returns a "no side  
> effects" error and I have to use !strict on all of it, which defeats  
> the purpose. And I'm pretty sure my code has side effects :)
>
>   
"code has no side effects" is a lint warning for:

foo && foo.bar && foo.bar()

the "longhand, jslint-approved way" of writing that would be

if(foo && foo.bar){
    foo.bar();
}

(note the absence of whitespace after 'if' and '{' and the presence of
whitespace around &&  ;) )

Just checked Komodo, and it (nor the textmate jslint-tooltip/checker I'm
using) breaks/warns on this:

(function(){
   
    var b = {
        bar: function(){
            return baz;
        }
    };
   
    b && b.bar && b.bar();
   
})();

Perhaps the existing jslint settings are too strict, but that is not the
discussion at hand.

Having a quick way to identify _style guideline_  breakage  seems really
cool to me. It would be trivial to make it a post-commit hook to reject
patches that don't _at least_ meet these guidelines, as we are supposed
to be doing manually already but clearly are not (with 5000+ offenses
produced from a non-difinitive parsing of the current codebase, ala
Shane's parser-thinger)

Regards,
Peter

> Mike Wilcox
> mwilcox at sitepen.com
> http://www.sitepen.com
> work: 650.968.8787 x218
> cell:	  214.697.4872
>
> On May 11, 2009, at 7:54 AM, Shane O'Sullivan wrote:
>
>   
>> jslint seems mostly to be just a target for controversy - while it has
>> many good points, we'd just end up talking about it for far too long
>> and nothing would get done on this.
>>
>> I think the style guidelines we have are pretty comprehensive, and
>> translating most of them into code shouldn't be too difficult.
>>
>> Alternatively, we could take a subset of jslint
>>
>> Shane
>>
>> 2009/5/11 Adam Peller <peller at gmail.com>:
>>     
>>> So, similar to the forum discussion, we should consider the cost of
>>> being 'different' and maintaining our own tools.  Is jslint a
>>> non-starter?
>>>
>>> On Mon, May 11, 2009 at 5:06 AM, Peter E Higgins <dante at dojotoolkit.org 
>>>       
>>>> wrote:
>>>> Sweet!
>>>>
>>>> Couple of things pop out at me (I'm looking at dojo/back.js now)
>>>>
>>>> * Whitespace around || statements: eg: var f = a||b||c should be  
>>>> var f =
>>>> a || b || c;
>>>> * Tabs/spaces after last character in line (we could probably strip
>>>> these pre-commit with sed or similar)
>>>> * if(foo){ }else{ } ... iirc we disallow unbracketed conditionals  
>>>> and
>>>> loops even for single line cases
>>>> * objects _should_ have space after the {}, eg: {display:"none"}  
>>>> should
>>>> be { display: "none" }, but I'm not sure that is mandatory.
>>>> * we're using document.write in that file. for the sake of  
>>>> consistency,
>>>> we could detect that and "suggest" using dojo.doc or something  
>>>> similar
>>>>
>>>> Those are [at least to me] the "important"/"common" things. It  
>>>> might be
>>>> too much to ask for check for a space/tab between a comment and a
>>>> keyword, eg:
>>>>
>>>> //summary:
>>>> //   Hi!
>>>>
>>>> should be
>>>>
>>>> //\tsummary:
>>>> //\t\tHi!
>>>>
>>>>
>>>> Otherwise, great stuff. Will take a peek at the patch and see what  
>>>> all
>>>> it is doing. Sorry I didn't get back to your inital email, but my
>>>> drafted repsonse was "yes, run with it"
>>>>
>>>> Regards,
>>>> Peter
>>>>
>>>> Shane O'Sullivan wrote:
>>>>         
>>>>> I've also put a copy of the report online at
>>>>> http://skynet.ie/~sos/misc/dojoCheckstyle/util/buildscripts/checkstyleReport.html
>>>>> for people to check out rather than having to apply the patch.
>>>>>
>>>>> Shane
>>>>>
>>>>> 2009/5/11 Shane O'Sullivan <shaneosullivan1 at gmail.com>:
>>>>>
>>>>>           
>>>>>> As usual, it's always the little things that get forgotten :-)   
>>>>>> Sorry
>>>>>> about that, the patch is there now.
>>>>>>
>>>>>> Shane
>>>>>>
>>>>>> 2009/5/11 Adam Peller <peller at gmail.com>:
>>>>>>
>>>>>>             
>>>>>>> Shane - I don't see the patch.  Can you post it again?
>>>>>>>
>>>>>>> I think it might be best to add this as a pre- (fatal) or
>>>>>>> post-processing (warning) svn-hook.
>>>>>>>
>>>>>>> On Sun, May 10, 2009 at 6:35 PM, Shane O'Sullivan
>>>>>>> <shaneosullivan1 at gmail.com> wrote:
>>>>>>>
>>>>>>>               
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Given that my first official check-in last week was riddled  
>>>>>>>> with style
>>>>>>>> guide violations, which I find it both difficult and boring to  
>>>>>>>> have to
>>>>>>>> look for (4 spaces looks VERY like a tab), I decided to have a  
>>>>>>>> go at
>>>>>>>> writing an extension to our build tools that does an optional  
>>>>>>>> check
>>>>>>>> for style guide violations.
>>>>>>>>
>>>>>>>> It obviously can't check for everything in the style guide,  
>>>>>>>> but it can
>>>>>>>> enforce many of the simpler guidelines.
>>>>>>>>
>>>>>>>> I've attached a patch file to http://bugs.dojotoolkit.org/ticket/9278
>>>>>>>> for people to look at.  It's extremely easy to use,  
>>>>>>>> instructions are
>>>>>>>> included in the ticket.
>>>>>>>>
>>>>>>>> What do people think?   Right now it only enforces a few  
>>>>>>>> rules, more
>>>>>>>> as a proof of concept than anything else.  However, it's  
>>>>>>>> extremely
>>>>>>>> easy to extend to enforce more.  Also, while some guidelines  
>>>>>>>> would be
>>>>>>>> too difficult to do in this manner, the fact that there's  
>>>>>>>> about 5000
>>>>>>>> violations in the toolkit right now of just these sample rules  
>>>>>>>> shows
>>>>>>>> that this can be a very valuable tool for tracking these issues.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Shane
>>>>>>>> _______________________________________________
>>>>>>>> dojo-contributors mailing list
>>>>>>>> dojo-contributors at mail.dojotoolkit.org
>>>>>>>> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>>>>>>>>
>>>>>>>>
>>>>>>>>                 
>>>>>>> _______________________________________________
>>>>>>> dojo-contributors mailing list
>>>>>>> dojo-contributors at mail.dojotoolkit.org
>>>>>>> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>>>>>>>
>>>>>>>
>>>>>>>               
>>>>> _______________________________________________
>>>>> dojo-contributors mailing list
>>>>> dojo-contributors at mail.dojotoolkit.org
>>>>> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>>>>>
>>>>>
>>>>>           
>>>> --
>>>> Peter E Higgins
>>>> Dojo Project Lead : http://dojotoolkit.org
>>>>
>>>> _______________________________________________
>>>> dojo-contributors mailing list
>>>> dojo-contributors at mail.dojotoolkit.org
>>>> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>>>>
>>>>         
>>> _______________________________________________
>>> dojo-contributors mailing list
>>> dojo-contributors at mail.dojotoolkit.org
>>> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>>>
>>>       
>> _______________________________________________
>> dojo-contributors mailing list
>> dojo-contributors at mail.dojotoolkit.org
>> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>>
>>
>>     
>
> _______________________________________________
> dojo-contributors mailing list
> dojo-contributors at mail.dojotoolkit.org
> http://mail.dojotoolkit.org/mailman/listinfo/dojo-contributors
>
>   


-- 
Peter E Higgins
Dojo Project Lead : http://dojotoolkit.org 



More information about the dojo-contributors mailing list