-
Notifications
You must be signed in to change notification settings - Fork 7
monkey-patched functions erase original function doc #62
Comments
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);
};
} workaroundsI 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 statementInstead 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 adviceUse 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 trickUse 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 endUse a pseudo-code doc block after both definitions, like we do for dom.byId():
fixAs 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:
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? |
No, that is fine if there is a workaround for the parser. What is the proper etiquette for updating comments for documentation purposes? |
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. |
PS: for some reason the "doc comment trick" I listed above didn't work in this case, but I checked in a similar fix:
|
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.
The text was updated successfully, but these errors were encountered: