Remove Marshal dependency #6
Conversation
|
Fully agreed on the removal of Marshal. Incidentally, I created an app after I wrote this library and that's when I first found out about the Decodable protocol. Moving forward with the Decodable protocol is the right move. I'll keep an eye on this as I can, but please ping me once it moves out of the draft phase. Again, thanks so much for stepping up! :) |
|
Hello again, @Chris-Perkins I've pushed a few changes into this (I added explanation in the PR). I'd be great if you could take a look and tell me what you think about it. Since you were already using structs for the results of the API calls, I thought it made sense to have them also for the request parameters (also, that's the swift way :P). This can help prevent bad encodings when making calls to the API. Unfortunately, it breaks source compatibility. |
There was a problem hiding this comment.
Overall, most of the problems related to problems you would find when you tested the problem queries (which I don't blame you for, there's a lot in this library). I left comments anywhere I noticed it which should speed up the process of identification, but can't guarantee this to be an exhaustive list.
| /// `language`: The language videos must be in to be returned. | ||
| let language: String? | ||
|
|
||
| /// `period`: The period to obtain data for. If this value is `.all`, Default: `.all`. |
There was a problem hiding this comment.
Looks like this may have been fragmented?
|
|
||
| /// `WebRequestKeys` define the web request keys for both resolving results and sending requests | ||
| /// for the New Twitch API. | ||
| internal struct WebRequestKeys { |
There was a problem hiding this comment.
Simply dear lord. It's necessary, just dear lord. :)
| public init(object: MarshaledObject) throws { | ||
| url = try object.value(for: Twitch.WebRequestKeys.url) | ||
| extensionId = try object.value(for: Twitch.WebRequestKeys.extensionId) | ||
| reportType = try object.value(for: Twitch.WebRequestKeys.type) |
There was a problem hiding this comment.
Taking special note here: the key value for the report type does not match up with the variable name. May need to consider renaming the variable for this and other such cases.
I.e. "reportType" != "type". "type" is the key for the web request value received from Twitch, and "reportType" is the local property value.
| url = try object.value(for: Twitch.WebRequestKeys.url) | ||
| extensionId = try object.value(for: Twitch.WebRequestKeys.extensionId) | ||
| reportType = try object.value(for: Twitch.WebRequestKeys.type) | ||
| startedAt = try? object.value(for: |
There was a problem hiding this comment.
I dug a bit deep for these web request values. Might need to consider doing a custom codable initialization or creating a DateRange object which contains startedAt and endedAt.
Twitch response:
"type" : ...,
"date_range" : {
"started_at" : "2018-03-13T00:00:00Z",
"ended_at" : "2018-06-13T00:00:00Z"
}
So you can see why the initialization here needs a special touch
| public init(object: MarshaledObject) throws { | ||
| id = try object.value(for: Twitch.WebRequestKeys.id) | ||
| ownerId = try object.value(for: Twitch.WebRequestKeys.userId) | ||
| ownerName = try object.value(for: Twitch.WebRequestKeys.userName) |
There was a problem hiding this comment.
Different property name vs web request key name, need to change variable name or do custom Codable initialization
| ownerName = try object.value(for: Twitch.WebRequestKeys.userName) | ||
| title = try object.value(for: Twitch.WebRequestKeys.title) | ||
| description = try object.value(for: Twitch.WebRequestKeys.description) | ||
| creationDate = try object.value(for: Twitch.WebRequestKeys.createdAt) |
There was a problem hiding this comment.
Different property name vs web request key name, need to change variable name or do custom Codable initialization
| title = try object.value(for: Twitch.WebRequestKeys.title) | ||
| description = try object.value(for: Twitch.WebRequestKeys.description) | ||
| creationDate = try object.value(for: Twitch.WebRequestKeys.createdAt) | ||
| publishedDate = try object.value(for: Twitch.WebRequestKeys.publishedAt) |
There was a problem hiding this comment.
Different property name vs web request key name, need to change variable name or do custom Codable initialization
| creationDate = try object.value(for: Twitch.WebRequestKeys.createdAt) | ||
| publishedDate = try object.value(for: Twitch.WebRequestKeys.publishedAt) | ||
| url = try object.value(for: Twitch.WebRequestKeys.url) | ||
| thumbnailURLString = try object.value(for: Twitch.WebRequestKeys.thumbnailURL) |
There was a problem hiding this comment.
Different property name vs web request key name, need to change variable name or do custom Codable initialization
| publishedDate = try object.value(for: Twitch.WebRequestKeys.publishedAt) | ||
| url = try object.value(for: Twitch.WebRequestKeys.url) | ||
| thumbnailURLString = try object.value(for: Twitch.WebRequestKeys.thumbnailURL) | ||
| viewSetting = try object.value(for: Twitch.WebRequestKeys.viewable) |
There was a problem hiding this comment.
Different property name vs web request key name, need to change variable name or do custom Codable initialization
|
We may also want to explicitly call out the date formatting method in the JSON Decoder; not sure if the default one is compatible with Twitch's format |
|
After looking at your comments it seems clear to me that it's a good idea to be explicit about the coding keys for each Codable type. I'm gonna open the API docs and review all the variable names/keys to make them match. I also saw there are some changes to the pagination you mention. When I'm done with params/result types I'll take a look into that. Maybe we can handle it using a single PaginationData object? (with generics) 🤷🏽♂️ As for the Encoder/Decoder date format, agreed. I'll take a look into it too. |
|
I was able to go through all the
|
|
Hi, So the v5 API endpoints were added as at the time I wrote the library, there were no such replacement API calls in the New Twitch API. I needed this endpoints specifically, so I added them for my own uses. No issues about keeping track of work items in this. Feel free to add them as you feel necessary. :) Thanks again for all the work, this is a large task, and really grateful you could take up the torch to make the library better. |
Marshal is a nice utility, but in the end the built-in
Decodableprotocol offers the same functionality. The performance gain Marshal is said to offer is marginal for this project, IMO, given that it mostly contains structs withIntandStringvalues in them. Instead, it adds the drag of having to maintain a dependency (plus it has not been updated in almost a year...) and a lot of boilerplate code.So in this PR (which is still a draft becase it needs some work and it relies on #5) I will:
Unmarshalingtype conform toCodableinit(object: MarshaledObject)initialisers and conformance toUnmarshalingValueTypeJSONDecoderJSONEncoderinstead of usingJSONSerializationURLRequestGetExtensionAnalyticsParamsGetGameAnalyticsParamsGetBitsLeaderboardParamsCreateClipParamsGetClipsParamsGetTopGamesParamsGetGamesParamsGetStreamsParamsGetFollowedStreamsParamsGetStreamsMetadataParamsCreateStreamMarkerParamsGetStreamMarkersParamsGetUsersParamsGetUsersFollowsParamsUpdateUserParamsGetVideosParamsperformAPIWebRequestmethod to make use of Codable objects rather than dictionariesInput on this is appreciated. Please let me know your thoughts.
This PR breaks source compatibility
In order to support Codable and make the API more Swifty (added type safety) by using custom structs instead of dictionaries I made the following changes:
performAPIWebRequestnow uses 3 generic parameters:WebRequestKeysare no longer. By usingJSONEncoderandJSONDecoderit is possible to specify thekeyEncodingStrategy. Therefore, as long as we keep the variable names as they should be in the API, we shouldn't need to store the keys. As a fallback in case there are strange keys we would like to modify, we could use custom CodingKeys. I moved the existing keys struct to its own file to maintain for future reference if needed.