-
Notifications
You must be signed in to change notification settings - Fork 164
Added support for Infinity and -Infinity #166
base: develop
Are you sure you want to change the base?
Conversation
if node.value.type is 'Identifier' and node.value.name is 'NaN' | ||
node.update("\"#{node.key.name}\": {\"$genghisType\": \"NaN\"}") | ||
# -Infinity is seen as a UnaryExp, so handle it with a special case | ||
if node.value.type is 'UnaryExpression' and node.value.argument.name is 'Infinity' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually know how esprima parse trees look for other unary expressions, but it seems like this line should be:
if node.value.type is 'UnaryExpression' and node.value.argument.type is 'Identifier' and node.value.argument.name is 'Infinity'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I guess the extra check on the argument type would be good practice.
Thanks for the fix :) Do you mind adding a test case for this as well? |
Oh hey, absolutely - I whipped this together because me and @9point6 where having a problem on our server. I guess for the test cases to pass i'll need to do the PHP version also? |
Ok, looks like those tests should pass now - just some silly typos really :s |
if node.value.type is 'Identifier' and node.value.name is 'NaN' | ||
node.update("\"#{node.key.name}\": {\"$genghisType\": \"NaN\"}") | ||
# -Infinity is seen as a UnaryExp, so handle it with a special case | ||
if node.value.type is 'UnaryExpression' and node.value.argument.type is 'Identifier' and node.value.argument.name is 'Infinity' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other unary expressions (e.g. ~
) so we should be checking the operator as well, and maybe whether it's a prefix operator. Otherwise things like this:
~Infinity
would get encoded as -Infinity
. This is getting long and heinous now, so I'd extract the conditions into a helper function :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richthegeek poke :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure in what case someone would do that exactly? Should be a simple change either way, but I'm at home now so I'll sort it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is when someone unintentionally does that. It should be a parse error, not an implicit thunk into -Infinity.
This allows Genghis to display and save Infinity values. This only works for the Ruby version at the moment, and there's probably some style issues with it... but it works!