Skip to content

Remove swift-corelibs-foundation duplicate implementations of Decimal and JSONEncoder/JSONDecoder #5017

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 1 commit into from
Jul 20, 2024

Conversation

parkera
Copy link
Contributor

@parkera parkera commented Jul 19, 2024

Also adjusts the Decimal tests so that we only test NSDecimalNumber here and rely on swift-foundation for Decimal.

Relies on swiftlang/swift-foundation#751

@@ -237,72 +237,36 @@ class TestJSONEncoder : XCTestCase {

func test_encodingOutputFormattingSortedKeys() throws {
let expectedJSON = try XCTUnwrap("""
{"2":"2","7":"7","25":"25","alice":"alice","bob":"bob","Charlie":"Charlie","中国":"中国","日本":"日本","韓国":"韓国"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sorting order changed in recent JSONEncoder implementations to be UTF8 byte based instead of a localized sort.

@@ -1286,8 +1286,8 @@ extension TestJSONSerialization {
}

func test_serialize_Decimal() {
XCTAssertEqual(try trySerialize([-Decimal.leastNonzeroMagnitude]), "[-0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now we have reverted to Darwin's Decimal behavior for the size of the mantissa, since it differed between platforms. We will revisit that after the Swift 6.0 release.

@parkera parkera force-pushed the parkera/replace_json_coders branch from 9839bda to 0454f9f Compare July 19, 2024 17:35
@@ -835,9 +800,9 @@ class TestJSONEncoder : XCTestCase {
try decode(type, value)
XCTFail("Decode of \(value) to \(type) should not succeed")
} catch DecodingError.dataCorrupted(let context) {
XCTAssertEqual(context.debugDescription, errorMessage)
// Pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really valuable to check the exact message here. Just needs to throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

XCTAssertThrowsError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks.

@parkera
Copy link
Contributor Author

parkera commented Jul 19, 2024

swiftlang/swift-foundation#751

@swift-ci please test

@@ -835,9 +800,9 @@ class TestJSONEncoder : XCTestCase {
try decode(type, value)
XCTFail("Decode of \(value) to \(type) should not succeed")
} catch DecodingError.dataCorrupted(let context) {
XCTAssertEqual(context.debugDescription, errorMessage)
// Pass
Copy link
Contributor

Choose a reason for hiding this comment

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

XCTAssertThrowsError?

@parkera parkera force-pushed the parkera/replace_json_coders branch from 0454f9f to acf9154 Compare July 19, 2024 17:43
@parkera
Copy link
Contributor Author

parkera commented Jul 19, 2024

@swift-ci test

@parkera
Copy link
Contributor Author

parkera commented Jul 19, 2024

swiftlang/swift-foundation#751

@swift-ci please test

@parkera
Copy link
Contributor Author

parkera commented Jul 19, 2024

@swift-ci please test windows

@parkera parkera merged commit 20967d3 into swiftlang:main Jul 20, 2024
2 of 3 checks passed
@parkera parkera deleted the parkera/replace_json_coders branch July 20, 2024 01:50
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