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

NSJSONSerialization JSONObjectWithData: implementation #54

Merged
merged 42 commits into from Dec 9, 2015
Merged

NSJSONSerialization JSONObjectWithData: implementation #54

merged 42 commits into from Dec 9, 2015

Conversation

argon
Copy link
Contributor

@argon argon commented Dec 7, 2015

Overview

This is a fully-tested RFC4627-compliant implementation of JSONObjectWithData(_:options:).

I would like to request feedback about whether this might be accepted in the near future, or conflicts with other plans before I invest more time in improving it.

Still needed

  • Documentation
  • Consistency improvements - my understanding of parsers evolved with the project
  • NSJSONReadingAllowFragments implementation
  • JSONObjectWithStream(_:options:) implementation - depends on unimplemented NSInputStream

Notes

The underlying parser is implemented as an internal type which would be well suited to reuse in possible-future API changes.

  • Trailing commas are allowed, to maintain compatibility with Darwin Foundation
  • Errors are defined as a Swift enum and closely follow Darwin Foundation error conditions

During initalization store the number type in the bit-field in the
same way that CFNumberCreate does-so.

NSNumber/CFNumber stores its value in a UInt64 (_pad) with associated type
information as part of the CFInfo bitfield. Initialization is performed
through _CFNumberInit which copies the value bytes into _pad but does
not store the type information.
...not the detection itself
guard let string = NSString(data: data, encoding: detectEncoding(data)) else {
throw NSJSONSerializationError.InvalidStringEncoding
}
return try JSONObjectWithString(string as String)
Copy link
Member

Choose a reason for hiding this comment

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

this is an implicit conversion that needs to be written as string._swiftObject

@parkera
Copy link
Member

parkera commented Dec 7, 2015

Just wanted to note that I think this is a great start, and is going in the direction we want.

@argon
Copy link
Contributor Author

argon commented Dec 8, 2015

@phausler I hadn't considered the return type conversions. Having looked at it now, I suppose the question here is whether to implement JSONDeserializer as purely as possible in Swift - with Bool, Double and String types, then returning Any instead of AnyObject. The final conversion to AnyObject could be performed explicitly before outputting from JSONObjectWithData to conform to existing Foundation API.

@phausler
Copy link
Member

phausler commented Dec 8, 2015

I would suspect that something along the lines of what NSPropertyListSerialization had to do will be required as well - see _expensivePropertyListConversion). The route of Any is a bit perilous because Dictionaries and have to then be Any to Any which Any is not Hashable. So some sort of container will have to be used that is Hashable (like the NSObject classes; they are already basically Hashable boxes)

@argon
Copy link
Contributor Author

argon commented Dec 8, 2015

I see now that NSSwiftRuntime has changed. I'm merging changes now and going to try converting JSONObjectWithString to return value types. I'll also try with object types as well to see which makes the most sense. There's lots of casting involved either way so it depends whether it's more valuable to be as "pure" as possible or to stick with object types given this is Foundation?

@argon argon changed the title [Feedback] NSJSONSerialization JSONObjectWithData: implementation NSJSONSerialization JSONObjectWithData: implementation Dec 8, 2015
@argon
Copy link
Contributor Author

argon commented Dec 8, 2015

@phausler I have merged in master and the NSJSONSerialization test suite passes again. I have opted to use Value types internally for now as hashValue is not implemented on NSString - meaning it's impossible to make the Object tests pass.

I did look at implementing hashValue but I see there is already some work on __NSCFType so I figured it would be landing soon?

@phausler
Copy link
Member

phausler commented Dec 8, 2015

hashValue for NSString should be relatively easy to implement; CFStringHashNSString is the thing that would do it and it just needs a few CF_SWIFT_CALLV additions.

@argon
Copy link
Contributor Author

argon commented Dec 8, 2015

I'll take a look at that then. I had assumed __NSCFType was intended to take care of it somehow but if NSString needs its own implementation I'll try to get it done

@argon
Copy link
Contributor Author

argon commented Dec 8, 2015

The dictionaries here are actually String: Any given the JSON format. That would solve the hashable problem, no?



#if DEPLOYMENT_RUNTIME_OBJC || os(Linux)
@testable import Foundation
Copy link
Member

Choose a reason for hiding this comment

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

@testable does not yet seem to be available on Linux targets

@phausler phausler merged commit e819ec4 into apple:master Dec 9, 2015
TestNSData(),
TestNSTimeZone(),
TestNSScanner(),
TestNSJSONSerialization(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated.

@argon argon deleted the feature/NSJSONSerialization-deserialization branch December 12, 2015 02:05
@naithar naithar mentioned this pull request Jan 20, 2017
atrick pushed a commit to atrick/swift-corelibs-foundation that referenced this pull request Jan 12, 2021
Update recommended toolchain to 2018-12-18-a
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.

None yet

5 participants