[dojo-contributors] No Side Effects Rejection

Peter E Higgins dante at dojotoolkit.org
Mon May 11 12:08:08 EDT 2009


Mike Wilcox wrote:
> You are suggesting it's a format violation which is fine, but that's  
> not what was throwing errors.
>
>   
I know, but my comment was that the issue was moot because the patch
should have been rejected based on the style guidelines alone.  The
warning was just a warning, and not an error, so it is just lint talking
to you. Our lint is clearly more strict than Komodo/Textmate/etc, and I
agree it is overly aggressive.

The issue is the ; is NOT required, and lint is complaining the "next
line" has no side effect. For example, the following commit attempt
failed. The number of Errors is equal to the number of excessive ;'s

Index: FileUploader.js
===================================================================
--- FileUploader.js     (revision 17515)
+++ FileUploader.js     (working copy)
@@ -18,7 +18,7 @@
 
        var mixin = function(o1,o2){
                // custom mixin
-               var o = {}, nm;
+               var o = {}, nm;;;;;;
                for(nm in o1){
                        if(dojo.isObject(o1[nm])){
                                o[nm] = mixin({}, o1[nm]);
sugr:form dante$ svn commit -m "refs #7340 - this is a test"
Sending        form/FileUploader.js
Transmitting file data .svn: Commit failed (details follow):
svn: MERGE request failed on '/src/dojox/trunk/form'
svn: 'pre-commit' hook failed with error output:
Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
Error at dojox/trunk/form/FileUploader.js:21 Code has no side effects
ERROR: Syntax errors. Checkin aborted. To override strict mode errors,
use "!strict" in comment.

sugr:form dante$

> As you proved though, it was the semi-colon in the wrong place. I  
> guess I can see "return" being invalid compared to "return;"
>
>   
I think it would have been fine if it were:

if(this._formNode){ return }

(note the lack of a ; after the }, which isn't an object or a var, its
the 'if' block)

Another interesting quirk here (don't quote me) return's cannot have a
newline on the 'return' line:

function bar(a){
    var b = 1;
    return
       a || b;
}

var x = bar(2); // expects 2
var y = bar(); // expects 1

console.log(x, y); // undefined, undefined

moving a || b to " return a || b; " works fine.

As it is all over our code, this is completely valid:

return someTruthy ?
    trueValue :
    falseValue;

> And as it turned out, a semi-colon after my try-catch was breaking  
> embed.Flash:
>
> try{..}catch(e){...); <----- that semi colon wouldn't allow a commit!!
>
>   
Yep, same case as above.
> I guess I would have to blame the validator for the improper message  
> on that second one. is it reading that as two statements?
>
>   
It is reading it as a line of code ";" that "does nothing", and "has no
side effects".

Please look at Shane's output, and ensure all modules you (all of us)
own pass the basic style guideline rules in place and already being
spotted safely.

http://skynet.ie/~sos/misc/dojoCheckstyle/util/buildscripts/checkstyleReport.html

Regards

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



More information about the dojo-contributors mailing list