Skip to content

Move optional metadata field so its not last#2909

Merged
sbc100 merged 2 commits intomasterfrom
fix_trailing_comma
Jun 11, 2020
Merged

Move optional metadata field so its not last#2909
sbc100 merged 2 commits intomasterfrom
fix_trailing_comma

Conversation

@sbc100
Copy link
Member

@sbc100 sbc100 commented Jun 11, 2020

To avoid the conditional trailing comma.

To avoid the conditional trailing comma.
@sbc100
Copy link
Member Author

sbc100 commented Jun 11, 2020

As alternative to #2908.. is it better? I'm not sure.

Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

This is fine too if you prefer it.

@dschuff
Copy link
Member

dschuff commented Jun 11, 2020

I guess we risk running into the problem again in the future if we add another optional at the end, but 🤷 .
I didn't even realize we had that commaFirst thing above. So much hackery for such a dumb design choice in JSON...

@sbc100
Copy link
Member Author

sbc100 commented Jun 11, 2020

I guess that idea is that we just need to make sure that optional fields are not last.. Not sure its worth a comment but perhaps worth adding to the tests to ensure the output is valid json? Given that this was not caught before emscripten integration.

@sbc100
Copy link
Member Author

sbc100 commented Jun 11, 2020

Added json parsing to tests

@sbc100 sbc100 merged commit 49f2443 into master Jun 11, 2020
@sbc100 sbc100 deleted the fix_trailing_comma branch June 11, 2020 23:18
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.

2 participants