Added support for non-NaNish base 16 numbers in jsonpath #328
Conversation
|
Hey, awesome thank you! Would you mind also signing our CLA? https://deepstream.io/info/community/cla/ |
| expect( record[ 0 ] ).toBe( undefined ); | ||
| expect( record[ pathName] ).toBe( 'value' ); | ||
| expect( jsonPath.get( record, pathName )).toBe( 'value' ); | ||
| expect( record[ pathName ]).toBe( 'value' ); |
src/record/json-path.js
Outdated
| @@ -124,6 +124,6 @@ function tokenize( path ) { | |||
| } | |||
|
|
|||
| return cache[ path ] = parts.map( function( part ) { | |||
| return !isNaN( part ) ? parseInt( part, 10 ) : part; | |||
| return part; | |||
There was a problem hiding this comment.
Maybe do
return cache[ path ] = parts.slice(0);There was a problem hiding this comment.
Hm actually, parts is created just a few lines above it, and isn't sent anywhere. So I guess I don't even need to copy it. I'll just cache[ path ] = parts
There was a problem hiding this comment.
Hm actually, parts is created just a few lines above it, and isn't sent anywhere. So I guess I don't even need to copy it. I'll just cache[ path ] = parts
Even better :)
| @@ -137,6 +137,16 @@ describe( 'objects are created from paths and their value is set correctly', fun | |||
| }); | |||
| }); | |||
|
|
|||
| it( 'even when the path is not NaNish and could be interpreted as a base 16 number', function() { | |||
| var record = {}; | |||
| let pathName = '0x02335'; | |||
There was a problem hiding this comment.
let fails build ( we can put a use strict at the top since this just tests though if you want )
There was a problem hiding this comment.
Ok. I changed to "var". Thanks!
|
Great thanks! |
|
@yasserf How often do you release versions? We could really use an updated version of the client lib. I'm not sure what to bump here, maybe it should be |
|
Hey @ralphtheninja, can do a quick release with that. Don't think this should be a major though, otherwise we will need to postpone it till 3.0 which is not due for a while. |
Check! A patch can be fine too. It depends on how you look at the change, since you could consider this a bug fix :) |
|
awesome, will plan a release to coincide with deepstream.io asap, just pending changelogs |
This is related to #327