Exposed new WPCOM service method to retrieve site verticals #66
Conversation
ghost
added
the
ghost
force-pushed
the
issue/10319-site-creation-verticals-service
branch
from
5a427a5 to
5aea34e
Compare
| func retrievalVerticals(request: SiteVerticalsRequest, completion: @escaping SiteVerticalsServiceCompletion) { | ||
|
|
||
| let endpoint = "verticals" | ||
| let path = self.path(forEndpoint: endpoint, withVersion: ._2_0) |
There was a problem hiding this comment.
At my last workplace we had a lot of issues due to the lack of API versioning and now I see the APi versions for ServiceRemoteWordPressComRESTApiVersion. This is great!
| let response = try self.decodeResponse(responseObject: responseObject) | ||
| completion(.success(response)) | ||
| } catch { | ||
| DDLogError("Failed to decode \([SiteVertical].self) : \(error.localizedDescription)") |
There was a problem hiding this comment.
I know it's not that important but just wondering: Any reason why you're logging the localizedDescription here but not in encodeRequestParameters?
There was a problem hiding this comment.
I like that the error which could arise from that method is closer to the completion handler for the failure case.
I don't feel strongly about it though. :)
| XCTAssertNoThrow(try JSONSerialization.jsonObject(with: encodedJSON, options: [])) | ||
| let serializedJSON = try! JSONSerialization.jsonObject(with: encodedJSON, options: []) | ||
|
|
||
| if let _ = serializedJSON as? [String : AnyObject] {} else { |
There was a problem hiding this comment.
Can we do
| if let _ = serializedJSON as? [String : AnyObject] {} else { | |
| guard let jsonDictionary = serializedJSON as [String : AnyObject] else { | |
| XCTFail("Failed to encode a proper JSON dictionary!") | |
| } |
and then remove let jsonDictionary = serializedJSON as! [String : AnyObject]
?
There was a problem hiding this comment.
I like it. I will push a change that takes care of this in each of these test methods.
There was a problem hiding this comment.
👍 Do you think we should do the same in line 53?
| // MARK: Multiple Verticals | ||
|
|
||
| func testSiteParsingOfMultipleVerticalsWorksAsExpected() { | ||
| XCTAssertTrue(!verticalsSUT.isEmpty) |
There was a problem hiding this comment.
why do we need to test that its not empty and that it is equal 5? shouldn't equal 5 be enough?
There was a problem hiding this comment.
You're not wrong. 😁 I will push a commit that takes care of this as well.
|
@stevebaranski , thank you so much for including me in this! Such a well written PR! |
|
@yaelirub thanks for the thoughtful feedback! I have pushed a commit that incorporates your suggestions. |
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Great stuff 👍 Tests passing both here and in the WPiOS wip branch.
3 participants
Description
Implements the service to fulfill the service implementation of this issue.
Testing
issue/10319-wordpresskit-wipand verify that it builds and that tests pass.