Handle JSONSerialization and NSNumber values

[joeosburn]

Here is one possible solution to the issue outlined here: #3

[Frizlab]

Thanks for the PR!
I can’t merge as-is because this drops Linux compatibility. Whenever I find the time, I’ll see if I can find a fix for Linux. For the time being, a simple #if os(Linux) should be enough.
If you could also add a unit test that’d be great.

At any rate, if you don’t have the time to do this, I’ll do it eventually.

[joeosburn]

Thanks @Frizlab, and thanks for your work on BSONSerialization.

I'll work on a a unit test and work on Linux support. Hopefully in the next couple of days.

[joeosburn]

@Frizlab OK, I made some updates. Please check it out when you have a moment and let me know if this is what you were looking for.

One note: While writing the unit test, I discovered that areBSONDocEqual had essentially the same issue, so I ended up duplicating the same bit of code in BSONClasses to deal with this. If there is a better way to solve this, please let me know. Thanks again.

[joeosburn]

By the way, the new test seems to pass just fine on Linux, as-is, so I don't think this problem is an issue on Linux.

[Frizlab]

Hello! Quick question: what version of Swift do you use? I tried at home and AFAICT the test does not fail on macOS without the fix with Swift 5.0.1.

I have reviewed the PR and can still merge your fix for Swift 4.2* if you need it (I’ll add the conditional Swift version check when we have confirmed it is a problem related to the Swift version).

EDIT: I dropped Swift 4.2 support as SimpleStream does not compile with Swift 4.2. But now I wonder why you got the test failure and I didn’t 🤔 Any idea?

[joeosburn]

Hmmm that is very strange. I am running Swift 5.0.1, macOS 10.14.6. (On Linux, swift 5.0.2) I went back and commented out the code and it definitely fails for me without the fix. I am not sure how to explain the difference between what we're seeing. Could it be related to a dependency or something else?

In any case - first, thanks for the improvement you did, and secondly, feel free to not merge it if you don't think it's necessary.

[Frizlab]

I cannot explain how you got the test failure, however I did find a way to have a failing test case when the fix is not implemented. Pushing soon.

[Frizlab]

Pushed. However now the test fails on Linux too! (As it should.)
Will try and find a fix that works on Linux too, then merge everything in master. Ideas welcome as usual :)

[Frizlab]

Pushed a thing that seems to be working on macOS and Linux.

Feel free to review and comment; I’ll leave you some time and merge eventually if you don’t come back to me :)

Incidentally I discovered this while working on this PR: https://bugs.swift.org/browse/SR-11430. Thanks for reporting the issue, it was very interesting (though the fix I got is disappointing IMHO).

[joeosburn]

This looks great! Thank you for the work you put into this and taking it to the next level.

The testing is odd. On Linux, I think it was passing for me because the test wasn't running, because it wasn't added to the XCTestManifests. However, on macOS, the test was running despite this. This is also the behavior I am experiencing in other projects. I am not sure why.

Good find on the swift bug!

For project I'm using BSONSerialization with, I had to pull BSONSerialization for unrelated reasons while I deal with other problems. I'm going to re-attempt using it hopefully in another month or two.

In any case, this looks great to me, and please take this as a from me. Thanks again.