Migrate to Swift 5 - #232
Conversation
diegoreymendez
left a comment
diegoreymendez
left a comment
There was a problem hiding this comment.
Hey @mindgraffiti ! I did a first pass on this PR and added some comments with recommended changes.
Please let me know if there's anything else.
| rating = try container.decode(Int.self, forKey: .rating) | ||
|
|
||
| let icons = try? container.decodeIfPresent([String: String].self, forKey: .icons) | ||
| let icons = ((try? container.decodeIfPresent([String: String].self, forKey: .icons)) as [String : String]??) |
There was a problem hiding this comment.
I think these two lines should be:
let icons = try? container.decodeIfPresent([String: String].self, forKey: .icons)
icon = icons?["2x"].flatMap(URL.init(string:))| let banners = try? container.nestedContainer(keyedBy: BannersKeys.self, forKey: .banners) | ||
|
|
||
| if let highRes = try? banners?.decodeIfPresent(String.self, forKey: .high) { | ||
| if let highRes = ((try? banners?.decodeIfPresent(String.self, forKey: .high)) as String??) { |
There was a problem hiding this comment.
I think this whole block should be:
if let highRes = try? banners?.decodeIfPresent(String.self, forKey: .high) {
banner = URL(string: highRes)
} else if let lowRes = try? banners?.decodeIfPresent(String.self, forKey: .low) {
banner = URL(string: lowRes)
} else {
banner = nil
}| let responseString = String(data: data, encoding: String.Encoding.utf8), | ||
| let parser = WordPressRSDParser(xmlString: responseString), | ||
| let endpoint = (try? parser.parsedEndpoint()), | ||
| let endpoint = (((try? parser.parsedEndpoint()) as String??)), |
There was a problem hiding this comment.
I think this line should be:
let xmlrpc = try? parser.parsedEndpoint(),And the immediate next line should be deleted. This one:
let xmlrpc = endpoint,
| rating = try container.decode(Int.self, forKey: .rating) | ||
|
|
||
| let icons = try? container.decodeIfPresent([String: String].self, forKey: .icons) | ||
| let icons = ((try? container.decodeIfPresent([String: String].self, forKey: .icons)) as [String : String]??) |
There was a problem hiding this comment.
is there a specific reason for casting to [String: String]? ? decodeIfPresent should return a [String: String] value or nil, depending if that value exists (and is not nil) or not, so if we leave this statement unchanged, can the following just be
icon = icons?["2x"].flatMap(URL.init(string:))?
or we could also:
if let icons = try container.decodeIfPresent([String: String].self, forKey: .icons) { icon = icons["2x"].flatMap(URL.init(string:)) }
There was a problem hiding this comment.
is there a specific reason for casting to
No there is not. I let Xcode do the migrating and overlooked how odd that change really was. Correcting it now :)
| let banners = try? container.nestedContainer(keyedBy: BannersKeys.self, forKey: .banners) | ||
|
|
||
| if let highRes = try? banners?.decodeIfPresent(String.self, forKey: .high) { | ||
| if let highRes = ((try? banners?.decodeIfPresent(String.self, forKey: .high)) as String??) { |
There was a problem hiding this comment.
Maybe we could do:
if let highRes = try banners?.decodeIfPresent(String.self, forKey: .high) { banner = URL(string: highRes) }
| let endpoint = (((try? parser.parsedEndpoint()) as String??)), | ||
| let xmlrpc = endpoint, | ||
| let xmlrpcURL = URL(string: xmlrpc) | ||
| else { |
There was a problem hiding this comment.
maybe the whole guard statement can be simplified like so?
guard let data = data,
let responseString = String(data: data, encoding: String.Encoding.utf8),
let parser = WordPressRSDParser(xmlString: responseString),
let xmlrpc = try? parser.parsedEndpoint(),
let xmlrpcURL = URL(string: xmlrpc)
else {
failure(WordPressOrgXMLRPCValidatorError.invalid as NSError)
return
}
|
@Gio2018 @diegoreymendez ready for a second review! I see what the To test the changed |
3 participants
Description
Fixes #79
Fixes #149
Test this PR by checking out wordpress-mobile/WordPress-iOS#13641
In this PR:
pod updateto get the latest acceptable changes for the internal (WordPress) podsOHclass prefix from code that uses OHHTTPStubsKnown issues
A warning will still remain in this project. Alamofire needs migrated from 4.x to 5.x in order to fully adopt Swift 5 in the project. A github issue will be filed for this.
Testing Details
bundle exec pod installbuild and run the project
Please check here if your pull request includes additional test coverage.