OpenLayers OpenLayers

Ticket #1439 (closed bug: fixed)

Opened 8 months ago

Last modified 8 months ago

OpenLayers.Style.createLiteral turns empty string into NaN

Reported by: tschaub Assigned to: tschaub
Priority: minor Milestone: 2.6 Release
Component: Style Version: 2.5
Keywords: Cc:
State: Complete

Description 

value = "";
value = isNaN(value) ? value : parseFloat(value);
value // NaN

value = "";
num = parseFloat(value);
value = isNaN(num) ? value : num;
value // ""

Attachments

emptyString.patch (0.6 kB) - added by tschaub on 03/13/08 20:30:15.
preserve empty strings
createLiteral.patch (2.4 kB) - added by tschaub on 03/16/08 16:26:19.
preserve empty string and strings starting with numbers

Change History

03/13/08 20:30:15 changed by tschaub

  • attachment emptyString.patch added.

preserve empty strings

03/13/08 20:30:56 changed by tschaub

  • state set to Review.

03/14/08 05:24:59 changed by ahocevar

  • state changed from Review to Commit.

Tim, thanks for this catch. The patch looks good. Please commit.

03/16/08 16:26:19 changed by tschaub

  • attachment createLiteral.patch added.

preserve empty string and strings starting with numbers

03/16/08 16:35:46 changed by tschaub

  • state changed from Commit to Review.

So, my patch was a bit greedy, turning strings that start with numbers (e.g. "50% correct") into numbers (e.g. 50). The !value check accounts for the empty string case while casting other strings with only numbers (and spaces) into numbers. If OL.String.format ever returns anything other than strings (e.g. null or undefined), this will still work.

Tests pass in FF. Would be good to confirm that IE isn't totally whacked.

(follow-up: ↓ 5 ) 03/16/08 16:46:20 changed by tschaub

And, fwiw, I still think isNaN is better used to determine whether or not a value is NaN - given the odd (and perhaps not browser consistent) behavior with "" and null (both of which return false).

http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Guide:Predefined_Functions:isNaN_Function

When it comes down to it, I think using a regex is more solid. 

function isFloat(value) {
    return /^([+-]?)(?=\d|\.\d)\d*(\.\d*)?([Ee]([+-]?\d+))?$/.test(value);
}

If you want to allow space padded numbers (as the isNaN(value) !value check does), then add them in the regex.

That said, I think this patch is good as is.

(in reply to: ↑ 4 ) 03/16/08 17:27:42 changed by tschaub

Replying to tschaub:

And, fwiw, I still think isNaN is better used to determine whether or not a value is NaN - given the odd (and perhaps not browser consistent) behavior with "" and null (both of which return false). When it comes down to it, I think using a regex is more solid.

See #1441

03/17/08 06:09:05 changed by ahocevar

  • state changed from Review to Commit.

Tests also pass in IE6. Patch looks good. Please commit.

Before switching to regular expressions, I would like to do some performance comparisons.

03/19/08 15:23:15 changed by tschaub

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(In [6553]) Fixing OpenLayers.Style.createLiteral to work with all strings. r=ahocevar (closes #1439)