-
Notifications
You must be signed in to change notification settings - Fork 108
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
Networking: Notifications Mark II #381
Networking: Notifications Mark II #381
Conversation
…ions-networking-mk2
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jleandroperez This is all great. I have some general comments, but I really couldn't find any issues in this PR.
|
||
/// Returns a String with the JSON Representation of the receiver. | ||
/// | ||
func toJSONEncoded() -> String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 !
|
||
/// WordPress.com Request Error | ||
/// | ||
public struct DotcomError: Error, Decodable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prolly should have done this a while ago...
let encoded = try! encoder.encode(parameters) | ||
|
||
return String(data: encoded, encoding: .utf8) | ||
return parameters.toJSONEncoded() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
|
||
/// WordPress.com Response Validator | ||
/// | ||
struct DotcomValidator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this approach with DotcomValidator
— preprocessing the response through this first via Remote
will give us the latitude to handle many different error conditions (and it's more efficient than letting a mapper error out).
/// | ||
public func updateReadStatus(_ notificationID: String, read: Bool, completion: @escaping (Error?) -> Void) { | ||
// Note: Isn't the API wonderful? | ||
let booleanFromPlanetMars = read ? Constants.readAsInteger : Constants.unreadAsInteger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
I'll document @bummytime @mindgraffiti thanks A LOT for the review!!!! |
Details:
Testing:
Ref. #19
cc @bummytime @mindgraffiti
Thanks in advance!!