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

NSNumber Initialization #28

Closed
cjmconie opened this issue Jun 24, 2019 · 3 comments
Closed

NSNumber Initialization #28

cjmconie opened this issue Jun 24, 2019 · 3 comments
Assignees
Labels

Comments

@cjmconie
Copy link
Collaborator

cjmconie commented Jun 24, 2019

Foundation's JSONSerialization.jsonObject(with:options:) transforms a Data type into an object of type Any. Since JSONs initialiser accepts an Any, this should work, but currently does not.

Note Issue 22 solved NSNull initialisation, which turned out to be a special case.

The primary issue is around the handling of NSNumber in the initialiser. JSONSerialization uses NSNumber to represent both numbers and booleans. This causes the following test to fail:

  1. Boolean transformation
func testInitializationFromJSONSerilizationBool() throws {
        
        let jsonData = "{\"boolean\": true}".data(using: .utf8)!
        let jsonObject = try JSONSerialization.jsonObject(with: jsonData, options: [])
        let json = try JSON(jsonObject)
        
        XCTAssertEqual(JSON.bool(true), json["boolean"]!)  // XCTAssertEqual failed: ("true") is not equal to ("1.0")
        
    }

In JSON's initialiser, the NSNumber is being downcast as Float, thus loosing it's boolean semantics. This is best explained here.

Here is SwiftyJSON's init for comparison. They also have a convenience function to determine whether an NSNumber is derived from a boolean.

  1. Double floating-point

The handling of NSNumbers's numbers are also leading to some odd results.

Similarly, when creating a JSON value with a fairly typical floating point value, say a latitude, results in precision loss:

let lat = JSON(18.471249902149363)  // 18.47125

Swift's Float represents a 32-bit floating-point number which results in precision loss in the cast above. This would not be the case if Double, which represents a 64-bit, were used.

My initial thinking was that we should change JSON's number associated value from Float to Double to accommodate double floating-point, especially since JavaScript's Number type also uses a 64-bit value. However now, looking at the SwiftyJSON implementation, maybe it should be done away with in favour of NSNumber.

What are your thoughts?

@zoul
Copy link
Collaborator

zoul commented Jun 30, 2019

Thank you, great suggestions! My thoughts:

  • The JSON spec doesn’t mandate any numeric representation. The conservative approach would be treating the numbers transparently, ie. using some big num machinery instead of (always problematic) floating point. The performance would probably go down, but I don’t think people care in this context. Main problem is that there is no standard bignum support in Swift (or is there?) and I’m not much keen on including an extra dependency.
  • As for Double vs. NSNumber, I’d like to stay away from NSStuff to keep as close to “pure Swift” as possible. (I don’t know much about this, but I got the impression that NSStuff tends to be more fragile on other platforms. But maybe that’s just irrational fear?) If there are no problems with Double that we know of, let’s keep the number as Double internally?
  • The proposed NSNumber number vs. bool solution seems fine. I don’t mind breaking backwards compatibility by replacing floatValue by doubleValue immediately, seems like users should be able to update their code pretty easily? Or we can add a deprecation warning for one release.

@cjmconie
Copy link
Collaborator Author

cjmconie commented Jul 1, 2019

Very happy to contribute 😁

  • PR 30 improves the handling of large numbers by using Double instead of Float. As to handling even larger floating point numbers, well that is still open for discussion. Maybe we should leave this open ended, and open a new issue if this ever comes up?

  • I think PR 30 solves this. JSON's number case is now associated with Double and NSNumber is only used internally as part of init(Any). This keeps the API Swifty. Regarding the impact on other platforms, I don't have any experience with this so I don't have much to contribute right now.

  • I think a breaking major version bump is fine. I have upgraded my code and it's pretty straightforward.

@zoul
Copy link
Collaborator

zoul commented Jul 1, 2019

Merged 🎉, thank you again for the contribution.

@zoul zoul closed this as completed Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants