Skip to content
This repository

ignore whitespace in filter values #334

Merged
merged 4 commits into from about 2 years ago

3 participants

Bart van den Eijnden Andreas Hocevar Marc Jansen
Bart van den Eijnden
Owner

Format.SLD tests still pass in FF and Safari, TIA for any review.

Bart van den Eijnden
Owner

I am a bit in doubt if this should be done for the generic Literal reader as well (which I did in the commit, however whitespace might have an actual meaning for comparing string values), it might only make sense for upperBoundary and lowerBoundary values instead. Feedback welcome.

Andreas Hocevar
Owner

A nice solution that makes the decision whether to trim or not based on the data type could look something like this:

var value = parseFloat(nodeValue);
if (isNaN(nodeValue)) {
    value = nodeValue;
}

Something similar could be done for Date types.

Bart van den Eijnden
Owner

@ahocevar Would it make sense to have OpenLayers.String.isNumeric ignore whitespace? That's very close to what you propose and more inline with the existing code or not?

Andreas Hocevar
Owner

@bartvde I don't remember the history of OpenLayers.String.isNumeric exactly, but I think there was a reason to not let it return true for " 1 ".

Bart van den Eijnden
Owner

@ahocevar so what about an optional argument allowWhitespace for OpenLayers.String.numericIf which is passed on to OpenLayers.String.isNumeric? Personally I prefer that approach instead of creating another similar code block in the Format itself.

Andreas Hocevar
Owner

@bartvde: Yes, I think a trimWhitespaceoption would make sense: if set to true, OpenLayers.String.numericIf would convert to number even if there is leading and trailing whitespace.

Bart van den Eijnden
Owner

This is now ready for review, TIA for any review. Tests pass in Safari and FF.

lib/OpenLayers/BaseTypes.js
((5 lines not shown))
192 193
      *
193 194
      * Returns:
194 195
      * {Number|String} a Number if the passed value is a number, a String
195 196
      *     otherwise. 
196 197
      */
197  
-    numericIf: function(value) {
  198
+    numericIf: function(value, trimWhitespace) {
  199
+        if (trimWhitespace === true) {
  200
+            value = value.replace(/^\s*|\s*$/g, "");
  201
+        }
198 202
         return OpenLayers.String.isNumeric(value) ? parseFloat(value) : value;
199 203
     }
1
Marc Jansen Collaborator
marcjansen added a note March 21, 2012

I think this changes exsting behaviour for non-numeric values. Before any leading/trailing whitespace would be returned, now only the trimmed value would come back. I suggest storing the trimmed value in another variable and return the original value for non-numeric values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/BaseTypes.html
@@ -233,6 +234,9 @@
233 234
             got = func(val);
234 235
             t.eq(got, exp, "'" + val + "' returns " + exp);
235 236
         }
  237
+
  238
+        // test the trimWhitespace option
  239
+        t.eq(OpenLayers.String.numericIf(" 27 ", true), 27, "if trimWhitespace is true, we expect to get a Number");
1
Marc Jansen Collaborator
marcjansen added a note March 21, 2012

Just to be double sure: can we test the remove whitespace option within the iteration above? The object we iterate over would then have anther key expectedWithTrim, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/Format/SLD/v1_0_0.html
((9 lines not shown))
  986
+            <sld:Name>US_Stat0_5cbbe918</sld:Name>
  987
+            <sld:Title>BMI&lt;25</sld:Title>
  988
+            <sld:FeatureTypeStyle>
  989
+                <sld:Name>name</sld:Name>
  990
+                <sld:Rule>
  991
+                    <sld:Title>BMI&lt;25</sld:Title>
  992
+                    <ogc:Filter>
  993
+                        <ogc:PropertyIsBetween>
  994
+                            <ogc:PropertyName>Hlt_st_BMI</ogc:PropertyName>
  995
+                            <ogc:LowerBoundary>
  996
+                                <ogc:Literal>
  997
+
  998
+
  999
+                                29.7
  1000
+
  1001
+
1
Marc Jansen Collaborator
marcjansen added a note March 21, 2012

Do we have all possible forms of whitespace in the XML? I am a little bit restricted in terms of viewing this diff, so if we already have spaces, \r, \n etc. ignore this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Marc Jansen
Collaborator

@bartvde I have added some comments you might want to have a look at.

Bart van den Eijnden
Owner

Thanks for a great review @marcjansen I've incorporated all your comments.

Could you look over my changes? TIA.

Andreas Hocevar
Owner

@marcjansen and I took another look. If you could add an expectedWithTrim for all cases to make sure that existing behavior does not change, we both agree that you can merge this.

Bart van den Eijnden bartvde merged commit 6b8c7a8 into from March 21, 2012
Bart van den Eijnden bartvde closed this March 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 4 unique commits by 1 author.

Mar 19, 2012
ignore whitespace in filter values 7a76983
instead, add a trimWhitespace option to OpenLayers.String.numericIf a…
…s discussed with @ahocevar
94bd370
Mar 21, 2012
incorporate review by @marcjansen 430d09e
add expectWithTrim to all test cases and protect numericIf against fa…
…ilure
2212924
This page is out of date. Refresh to see the latest.
9  lib/OpenLayers/BaseTypes.js
@@ -189,13 +189,18 @@ OpenLayers.String = {
189 189
      * 
190 190
      * Parameters:
191 191
      * value - {String}
  192
+     * trimWhitespace - {Boolean}
192 193
      *
193 194
      * Returns:
194 195
      * {Number|String} a Number if the passed value is a number, a String
195 196
      *     otherwise. 
196 197
      */
197  
-    numericIf: function(value) {
198  
-        return OpenLayers.String.isNumeric(value) ? parseFloat(value) : value;
  198
+    numericIf: function(value, trimWhitespace) {
  199
+        var originalValue = value;
  200
+        if (trimWhitespace === true && value != null && value.replace) {
  201
+            value = value.replace(/^\s*|\s*$/g, "");
  202
+        }
  203
+        return OpenLayers.String.isNumeric(value) ? parseFloat(value) : originalValue;
199 204
     }
200 205
 
201 206
 };
8  lib/OpenLayers/Format/Filter/v1.js
@@ -27,7 +27,7 @@ OpenLayers.Format.Filter.v1 = OpenLayers.Class(OpenLayers.Format.XML, {
27 27
         xlink: "http://www.w3.org/1999/xlink",
28 28
         xsi: "http://www.w3.org/2001/XMLSchema-instance"
29 29
     },
30  
-    
  30
+
31 31
     /**
32 32
      * Property: defaultPrefix
33 33
      */
@@ -180,18 +180,18 @@ OpenLayers.Format.Filter.v1 = OpenLayers.Class(OpenLayers.Format.XML, {
180 180
             },
181 181
             "Literal": function(node, obj) {
182 182
                 obj.value = OpenLayers.String.numericIf(
183  
-                    this.getChildValue(node));
  183
+                    this.getChildValue(node), true);
184 184
             },
185 185
             "PropertyName": function(node, filter) {
186 186
                 filter.property = this.getChildValue(node);
187 187
             },
188 188
             "LowerBoundary": function(node, filter) {
189 189
                 filter.lowerBoundary = OpenLayers.String.numericIf(
190  
-                    this.readers.ogc._expression.call(this, node));
  190
+                    this.readers.ogc._expression.call(this, node), true);
191 191
             },
192 192
             "UpperBoundary": function(node, filter) {
193 193
                 filter.upperBoundary = OpenLayers.String.numericIf(
194  
-                    this.readers.ogc._expression.call(this, node));
  194
+                    this.readers.ogc._expression.call(this, node), true);
195 195
             },
196 196
             "Intersects": function(node, obj) {
197 197
                 this.readSpatial(node, obj, OpenLayers.Filter.Spatial.INTERSECTS);
47  tests/BaseTypes.html
@@ -201,28 +201,30 @@
201 201
     
202 202
     function test_Number_numericIf(t) {
203 203
         var cases = [
204  
-            {value: "3", expect: 3},
205  
-            {value: "+3", expect: 3},
206  
-            {value: "-3", expect: -3},
207  
-            {value: "3.0", expect: 3},
208  
-            {value: "+3.0", expect: 3},
209  
-            {value: "-3.0", expect: -3},
210  
-            {value: "6.02e23", expect: 6.02e23},
211  
-            {value: "+1.0e-100", expect: 1e-100},
212  
-            {value: "-1.0e+100", expect: -1e100},
213  
-            {value: "1E100", expect: 1e100},
214  
-            {value: null, expect: null},
215  
-            {value: true, expect: true},
216  
-            {value: false, expect: false},
217  
-            {value: undefined, expect: undefined},
218  
-            {value: "", expect: ""},
219  
-            {value: "3 ", expect: "3 "},
220  
-            {value: " 3", expect: " 3"},
221  
-            {value: "1e", expect: "1e"},
222  
-            {value: "1+e", expect: "1+e"},
223  
-            {value: "1-e", expect: "1-e"}
  204
+            {value: "3", expect: 3, expectWithTrim: 3},
  205
+            {value: "+3", expect: 3, expectWithTrim: 3},
  206
+            {value: "-3", expect: -3, expectWithTrim: -3},
  207
+            {value: "3.0", expect: 3, expectWithTrim: 3},
  208
+            {value: "+3.0", expect: 3, expectWithTrim: 3},
  209
+            {value: "-3.0", expect: -3, expectWithTrim: -3},
  210
+            {value: "6.02e23", expect: 6.02e23, expectWithTrim: 6.02e23},
  211
+            {value: "+1.0e-100", expect: 1e-100, expectWithTrim: 1e-100},
  212
+            {value: "-1.0e+100", expect: -1e100, expectWithTrim: -1e100},
  213
+            {value: "1E100", expect: 1e100, expectWithTrim: 1e100},
  214
+            {value: null, expect: null, expectWithTrim: null},
  215
+            {value: true, expect: true, expectWithTrim: true},
  216
+            {value: false, expect: false, expectWithTrim: false},
  217
+            {value: undefined, expect: undefined, expectWithTrim: undefined},
  218
+            {value: "", expect: "", expectWithTrim: ""},
  219
+            {value: "3 ", expect: "3 ", expectWithTrim: 3},
  220
+            {value: " 3", expect: " 3", expectWithTrim: 3},
  221
+            {value: "1e", expect: "1e", expectWithTrim: "1e"},
  222
+            {value: "1+e", expect: "1+e", expectWithTrim: "1+e"},
  223
+            {value: "1-e", expect: "1-e", expectWithTrim: "1-e"},
  224
+            {value: " 27 ", expect: " 27 ", expectWithTrim: 27},
  225
+            {value: " abc ", expect: " abc ", expectWithTrim: " abc "}
224 226
         ];
225  
-        t.plan(cases.length);
  227
+        t.plan(cases.length*2);
226 228
         
227 229
         var func = OpenLayers.String.numericIf;
228 230
         var obj, val, got, exp;
@@ -232,6 +234,9 @@
232 234
             exp = obj.expect;
233 235
             got = func(val);
234 236
             t.eq(got, exp, "'" + val + "' returns " + exp);
  237
+            got = func(val, true);
  238
+            exp = obj.expectWithTrim;
  239
+            t.eq(got, exp, "'" + val + "' returns " + exp + " with trimWhitespace true");
235 240
         }
236 241
     }
237 242
    
54  tests/Format/SLD/v1_0_0.html
@@ -523,6 +523,14 @@
523 523
         
524 524
     }
525 525
 
  526
+    function test_whitespace(t) {
  527
+        t.plan(1);
  528
+        var xml = readXML("propertyisbetweenwhitespace.sld");
  529
+        var output = new OpenLayers.Format.SLD().read(xml);
  530
+        var filter = output.namedLayers['geonode:US_Stat0'].userStyles[0].rules[0].filter;
  531
+        t.eq(filter.lowerBoundary, 29.7, "whitespace ignored in values and value transformed to number");
  532
+    }
  533
+
526 534
     function test_label_LinePlacement(t) {
527 535
         t.plan(1);
528 536
         var format = new OpenLayers.Format.SLD.v1_0_0({
@@ -970,5 +978,51 @@
970 978
     </sld:FeatureTypeStyle>
971 979
 </sld:UserStyle>
972 980
 --></div>
  981
+<div id="propertyisbetweenwhitespace.sld"><!--
  982
+<sld:StyledLayerDescriptor xmlns="http://www.opengis.net/sld" xmlns:sld="http://www.opengis.net/sld" xmlns:ogc="http://www.opengis.net/ogc" xmlns:gml="http://www.opengis.net/gml" version="1.0.0">
  983
+    <sld:NamedLayer>
  984
+        <sld:Name>geonode:US_Stat0</sld:Name>
  985
+        <sld:UserStyle>
  986
+            <sld:Name>US_Stat0_5cbbe918</sld:Name>
  987
+            <sld:Title>BMI&lt;25</sld:Title>
  988
+            <sld:FeatureTypeStyle>
  989
+                <sld:Name>name</sld:Name>
  990
+                <sld:Rule>
  991
+                    <sld:Title>BMI&lt;25</sld:Title>
  992
+                    <ogc:Filter>
  993
+                        <ogc:PropertyIsBetween>
  994
+                            <ogc:PropertyName>Hlt_st_BMI</ogc:PropertyName>
  995
+                            <ogc:LowerBoundary>
  996
+                                <ogc:Literal>
  997
+
  998
+
  999
+                                	29.7
  1000
+
  1001
+
  1002
+                            </ogc:Literal>
  1003
+                            </ogc:LowerBoundary>
  1004
+                            <ogc:UpperBoundary>
  1005
+                                <ogc:Literal>
  1006
+
  1007
+
  1008
+                                 36.2
  1009
+
  1010
+
  1011
+                            </ogc:Literal>
  1012
+                            </ogc:UpperBoundary>
  1013
+                        </ogc:PropertyIsBetween>
  1014
+                    </ogc:Filter>
  1015
+                    <sld:PolygonSymbolizer>
  1016
+                        <sld:Fill>
  1017
+                            <sld:CssParameter name="fill">#C0F58C</sld:CssParameter>
  1018
+                        </sld:Fill>
  1019
+                        <sld:Stroke/>
  1020
+                    </sld:PolygonSymbolizer>
  1021
+                </sld:Rule>
  1022
+            </sld:FeatureTypeStyle>
  1023
+        </sld:UserStyle>
  1024
+    </sld:NamedLayer>
  1025
+</sld:StyledLayerDescriptor>
  1026
+--></div>
973 1027
 </body> 
974 1028
 </html> 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.