Migrate to Swift 4 #131
Migrate to Swift 4 #131
Conversation
loudmouth
force-pushed
the
improvement/swift-4
branch
2 times, most recently
from
efae1dc to
e6b5e0f
Compare
Write extensions to decode arrays and dictionarys Reinstate self made hex string transform
loudmouth
force-pushed
the
improvement/swift-4
branch
from
e6b5e0f to
ee46f83
Compare
| } | ||
|
|
There was a problem hiding this comment.
I kinda liked the new line here ... :(
| let container = try decoder.container(keyedBy: CodingKeys.self) | ||
| fileName = try container.decode(String.self, forKey: .fileName) | ||
| contentType = try container.decode(String.self, forKey: .contentType) | ||
| details = try container.decode(Details.self, forKey: .details) |
There was a problem hiding this comment.
Does this handle decoding the imageInfo too? Or would you need a init on the Details object too?
There was a problem hiding this comment.
It indeed does handle the imageInfo! All decoding decodes and properties that are also Decodable which ImageInfo is.
Sources/Contentful/Client.swift
Outdated
| let map = Map(mappingType: .fromJSON, JSON: json, context: localizationContext) | ||
|
|
||
| let jsonDecoder = Client.jsonDecoderWithoutContext // JSONDecoder() | ||
| jsonDecoder.userInfo[LocalizableResource.localizationContextKey] = localizationContext | ||
|
|
||
| // Use `Mappable` failable initialzer to optional rather throwing `ImmutableMappable` initializer |
There was a problem hiding this comment.
Not sure if this comment is still acurate: Do you still use a mappable?
Sources/Contentful/Client.swift
Outdated
| } catch _ { | ||
| completion(.error(SDKError.unparseableJSON(data: data, errorMessage: ""))) | ||
| completion(Result.error(SDKError.unparseableJSON(data: data, errorMessage: ""))) |
There was a problem hiding this comment.
I see that this general catch was in here before, but I am wondering if this can also indicate another error, something which is not a DecodingError but an Out of memory. So maybe then all the unparseableJSON-Error might be misleading? (It is actually parsable, but something else went wrong ...
There was a problem hiding this comment.
i have consolidated to one catch statement now.
|
|
||
| super.init(dateFormatter: formatter) | ||
| } | ||
| } |
|
|
||
|
|
||
| // Use CGColor instead of UIColor to enable cross-platform compatibility: macOS, iOS, tvOS, watchOS. | ||
| internal extension CGColor { |
There was a problem hiding this comment.
Does coco/darwin handle alpha components? If so, should they be part of your color model?
| var result = left | ||
| right.forEach { (key, value) in result[key] = value } | ||
| return result | ||
| } | ||
|
|
||
| internal func +<K: Hashable, V> (left: [K: V], right: [K: V]) -> [K: V] { | ||
| internal func +<K, V> (left: [K: V], right: [K: V]) -> [K: V] { |
There was a problem hiding this comment.
I fear this might not the right implementation of +. Also I feel I stumbled upon this before: Is left passed in as a reference or as a copy?
If it is a copy (aka changing it in here, does not change the variable outside) then I take back my comment and bow over in front of swift.
There was a problem hiding this comment.
it is a copy in swift since dictionaries are structs. when you pass one in, it is copied. we've had this conversation before ;-)
|
|
||
|
|
||
| // Use CGColor instead of UIColor to enable cross-platform compatibility: macOS, iOS, tvOS, watchOS. | ||
| internal extension CGColor { |
There was a problem hiding this comment.
Also: I would love to see tests about this...
| expect(dogs.count).to(equal(1)) | ||
| expect(dogs.first?.name).to(equal("Jake")) | ||
| case .error(let error): | ||
| fail("Should not throw an error \(error)") |
There was a problem hiding this comment.
does failing fullfill the expectation too? Or would it wait for ever in here now?
There was a problem hiding this comment.
No, the expectation is fulfilled outside of the switch: no matter the case, it is fulfilled.
| QueryTests.client.fetchMappedEntries(with: query).then { citiesResponse in | ||
| QueryTests.client.fetchMappedEntries(with: query) { result in | ||
| switch result { | ||
| case .success(let citiesResponse): | ||
| let cities = citiesResponse.items |
There was a problem hiding this comment.
Looks like you missed an indentation here.
No description provided.