monkey-patched functions erase original function doc

[kitsonk]

The parser is not handling dojo/number::round() properly. The code is detecting an IE issue with rounding and then overrides it's own method to fix it. It appears js-doc-parse is taking this as the "documented" method, and ignoring the previous full definition of the method.

[wkeese]

So, you are talking about this monkey-patching code:

if((0.9).toFixed() == 0){
    // (isIE) toFixed() bug workaround: Rounding fails on IE when most significant digit
    // is just after the rounding place and is >=5
    var round = number.round;
    number.round = function(v, p, m){
        var d = Math.pow(10, -p || 0), a = Math.abs(v);
        if(!v || a >= d || a * Math.pow(10, p + 1) < 5){
            d = 0;
        }
        return round(v, p, m) + (v > 0 ? d : -d);
    };
}

workarounds

I just dealt with a bunch of problems like that, like for example when dojox/mvc overrides some methods in dijit. Some possible workarounds are:

if statement

Instead of monkey patching the function, just use an if() statement:

number.round = function(/*Number*/ value, /*Number?*/ places, /*Number?*/ increment){
    // summary: ...
    if((0.9).toFixed() == 0){
        // (isIE) toFixed() bug workaround: Rounding fails on IE when most significant digit
        // is just after the rounding place and is >=5
        var d = Math.pow(10, -places || 0), a = Math.abs(value);
        if(!value || a >= d || a * Math.pow(10, places + 1) < 5){
            d = 0;
        }
    }
    var factor = 10 / (increment || 10);
    return (factor * +value).toFixed(places) / factor; // Number
};

It's dead simple, but degrades performance on modern browser. Whether or not the performance change is significant is a different question.

Also note that the bug-test should really be in a has.add() block so we can filter it out for webkit-only builds.

around advice

Use dojo/aspect.around() to modify the behavior of round(). Thankfully the parser doesn't try to interpret around advice.

if((0.9).toFixed() == 0){
    // (isIE) toFixed() bug workaround: Rounding fails on IE when most significant digit
    // is just after the rounding place and is >=5
    aspect.around(number, "round", function(round){
        return function(v, p, m){
            var d = Math.pow(10, -p || 0), a = Math.abs(v);
            if(!v || a >= d || a * Math.pow(10, p + 1) < 5){
                d = 0;
            }
            return round(v, p, m) + (v > 0 ? d : -d);
        };
    });
}

This seems like the most legible solution, working around the parser problem and improving code readability at the same time, but probably we don't want to introduce a dependency from dojo/number to dojo/aspect.

doc comment trick

Use doc-comments to trick the parser:

if((0.9).toFixed() == 0){
    // (isIE) toFixed() bug workaround: Rounding fails on IE when most significant digit
    // is just after the rounding place and is >=5
    var round = number.round;
    number.round = /*===== number.round || =====*/ function(v, p, m){
        var d = Math.pow(10, -p || 0), a = Math.abs(v);
        if(!v || a >= d || a * Math.pow(10, p + 1) < 5){
            d = 0;
        }
        return round(v, p, m) + (v > 0 ? d : -d);
    };
}

Probably the best solution for this particular case.

pseudo function in doc block at the end

Use a pseudo-code doc block after both definitions, like we do for dom.byId():

if(has("ie")){
    dom.byId = function(id, doc){
        ...
    };
}else{
    dom.byId = function(id, doc){
        ...
    };
}
/*=====
 dom.byId = function(id, doc){
     // summary:
     //     Returns DOM node with matching `id` attribute or `null`
     //     if not found. If `id` is a DomNode, this function is a no-op.
     ...
 };
 =====*/

fix

As to fixing the issue, not sure what you want the parser to do? Ignore any code inside of if() statements or ternaries? It just seems complicated/dangerous. For example, consider this code from json.js:

    return {
        // summary:
        //      Functions to parse and serialize JSON

        parse: has("json-parse") ? JSON.parse : function(str, strict){
            // summary:
            //      Parses a [JSON](http://json.org) string to return a JavaScript object.
            ...
        },

Imagine if the ternary listed to inlined functions. How does the parser know which definition to use?

In any case, seems like we should workaround this issue in the code for now and possibly improve the parser later, since there are lots of other parser bugs that we can't workaround. Agreed?

[kitsonk]

No, that is fine if there is a workaround for the parser. What is the proper etiquette for updating comments for documentation purposes?

[wkeese]

I've just been updating them, without any "etiquette". I don't think anyone minds, or that it's practical to coordinate every documentation fix.

[wkeese]

PS: for some reason the "doc comment trick" I listed above didn't work in this case, but I checked in a similar fix:

   // Use "doc hint" so the doc parser ignores this new definition of round(), and uses the one above.
   /*===== number.round = round; =====*/