Skip to content
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

Output null instead of undefined with Json.undefined serialization #1737

Merged
merged 1 commit into from
May 16, 2017

Conversation

tchaloupka
Copy link
Contributor

@tchaloupka tchaloupka commented Apr 8, 2017

Fixes #1735

@@ -2010,7 +2016,6 @@ void writeJsonString(R, bool pretty = false)(ref R dst, in Json json, size_t lev
dst.put('{');
bool first = true;
foreach (string k, ref const Json e; json.byKeyValue) {
if( e.type == Json.Type.undefined ) continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this because it will not serialize the attribute at all and I think, that this should be handled by @optional attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect because Json.undefined literally means the key should not be present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded. This was done to be consistent with the ECMA script spec, from which the undefined value originates.

@tchaloupka tchaloupka force-pushed the fix_#1735 branch 2 times, most recently from e5975db to 9baa162 Compare May 14, 2017 10:08
revert change

fix unitest
@s-ludwig s-ludwig merged commit d51beec into vibe-d:master May 16, 2017
s-ludwig added a commit that referenced this pull request Sep 2, 2017
Output null instead of undefined with Json.undefined serialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants