New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nan -> null not undefined #1548

Merged
merged 3 commits into from Aug 22, 2016

Conversation

Projects
None yet
2 participants
@John-Colvin
Contributor

John-Colvin commented Aug 13, 2016

should add some more tests, but I believe this is a necessary change. Either this or throw an exception on serialising a nan. undefined is not a valid json literal and should never appear in any JSON output.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 18, 2016

Member

Thanks, this should have been fixed long ago. Fixes #1442 and #958. There is also a related PR that I lost track of (#1323, sorry @rjmcguire for having missed your comment). Can you integrate that change here, too (i.e. the same change, but in the JsonStringSerializer)?

Member

s-ludwig commented Aug 18, 2016

Thanks, this should have been fixed long ago. Fixes #1442 and #958. There is also a related PR that I lost track of (#1323, sorry @rjmcguire for having missed your comment). Can you integrate that change here, too (i.e. the same change, but in the JsonStringSerializer)?

@John-Colvin

This comment has been minimized.

Show comment
Hide comment
@John-Colvin

John-Colvin Aug 18, 2016

Contributor

ok, done. I guess I need to test that, not sure where

Contributor

John-Colvin commented Aug 18, 2016

ok, done. I guess I need to test that, not sure where

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 18, 2016

Member

Thanks! assert(serializeToJsonString(double.nan) == "null"); should be a valid test (I'd place it directly below the JsonStringSerializer struct).

Member

s-ludwig commented Aug 18, 2016

Thanks! assert(serializeToJsonString(double.nan) == "null"); should be a valid test (I'd place it directly below the JsonStringSerializer struct).

@John-Colvin

This comment has been minimized.

Show comment
Hide comment
@John-Colvin

John-Colvin Aug 22, 2016

Contributor

done

Contributor

John-Colvin commented Aug 22, 2016

done

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 22, 2016

Member

LGTM!

Member

s-ludwig commented Aug 22, 2016

LGTM!

@s-ludwig s-ludwig merged commit 62f5c65 into vibe-d:master Aug 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment