PDA

View Full Version : [FIXED-216][3.x/2.x] Regression - Ext.num('') returns 0



mystix
31 Aug 2009, 7:07 AM
just discovered a tiny regression bug -- Ext.num('') now returns 0.

just a little tweak is required to resolve this:


Ext.apply(Ext, {
num: function(v, defaultValue) {
v = Number(v === null || String(v).replace(/^\s+/, '') === '' /* in the interests of speed, just trim the head and avoid using Ext.isEmpty() and String.prototype.trim() */ || Ext.isBoolean(v) ? NaN : v);

return isNaN(v) ? defaultValue : v;
}
});

this bug affects both the 3.x and 2.x branches.

evant
1 Sep 2009, 8:49 AM
Fix applied to svn in rev #5270 for patch release 3.0.2.

mystix
1 Sep 2009, 6:01 PM
unfortunately that doesn't take care of


Ext.num(' '); // returns 0
Ext.num(' '); // returns 0

i.e. a string of spaces still returns 0 with the present solution in SVN.


which is why i suggested the whitespace replacement strategy:
i found a shorter method of dealing with a string containing nothing but whitespace (of which the empty string is a special case):
(also recommend changing the Ext.isEmpty() check back to a strict nullity check -- see comments below)


Ext.apply(Ext, {
num: function(v, defaultValue) {
v = Number(v === null || /^\s*$/.test(v) || Ext.isBoolean(v) ? NaN : v);
return isNaN(v) ? defaultValue: v;
}
});



a nice(???) / unexpected side-effect of the Ext.isEmpty() strategy in SVN is that it takes care of Ext.num([]), which otherwise returns 0, while Ext.num([1]) returns 1 because Number([1]) === 1, which is cool(??), but not expected. additionally, it also incurs the overhead of an additional function call.

evant
1 Sep 2009, 6:23 PM
I'm not sure whether Number([1]) evaluating to 1 is a good thing.

In fact I think maybe adding a second trim parameter to isEmpty might be useful.

mystix
1 Sep 2009, 6:28 PM
by adding an additional allowEmptyArray boolean parameter?

i'm not sure if Ext.num() should even bother handling the empty array situation -- i merely happened upon that result by accident -- which is why i recommend switching the isEmpty() check back to a strict nullity check.

evant
1 Sep 2009, 6:36 PM
I was thinking more along the lines of empty string, so that isEmpty would automatically trim it. But then again, it's probably not that useful.

mystix
1 Sep 2009, 6:55 PM
eureka...

try this then:


Ext.apply(Ext, {
isEmpty : function(v, allowBlank) {
return v === null || v === undefined || ((Ext.isArray(v) && !v.length)) ||
(!allowBlank ? /^\s*$/.test(v) : false);
}
});

any empty string / a string which consists only of whitespace will be treated as empty.

might want to be careful with that though -- some ppl might actually want a non-empty blank string to be treated as a truthy value. might be good to get a consensus on this.

evant
1 Sep 2009, 7:02 PM
That's why I'm saying we could add another param to indicate whether it should trim the value.

mystix
1 Sep 2009, 7:07 PM
my bad... :">

coffee underdose. my morning cuppa's still sitting pretty on my desk. :s