Skip to content

Make JSON pretty-printing consistent with Darwin. #1205

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

Merged
merged 2 commits into from
Sep 9, 2017
Merged

Make JSON pretty-printing consistent with Darwin. #1205

merged 2 commits into from
Sep 9, 2017

Conversation

mattrajca
Copy link
Contributor

@mattrajca mattrajca commented Sep 7, 2017

On macOS, pretty-printing JSON results in a space before the colon, e.g.:

{
  "key" : 4
}

Addresses SR-5570.

On macOS, pretty-printing JSON results in a space before the colon, e.g.:

	{
	  "key" : 4
	}
@@ -989,6 +989,7 @@ extension TestJSONSerialization {
("test_serialize_dictionaryWithDecimal", test_serialize_dictionaryWithDecimal),
("test_serializeDecimalNumberJSONObject", test_serializeDecimalNumberJSONObject),
("test_serializeSortedKeys", test_serializeSortedKeys),
("test_colonPrettyPrintingMatchesDarwin", test_colonPrettyPrintingMatchesDarwin),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call this test_serializePrettyPrinted() please? In time it should be expanded to test more than just the colon positioning, plus it is not actually testing Darwin compatibility, rather a string which currently matches Darwin. In other words, if Darwin is changed this test will not automatically start failing.

@ianpartridge
Copy link
Contributor

@spevans I thought you already fixed this recently but it appears not?

@spevans
Copy link
Contributor

spevans commented Sep 8, 2017

I was going to in my PR that fixed all of the numeric types but left it out in the end.

@mattrajca
Copy link
Contributor Author

@ianpartridge : Renamed.

@ianpartridge
Copy link
Contributor

@swift-ci please test and merge

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.

4 participants