Consolidates XPCError generation logic
#62
Conversation
Adds two static functions to `XPCError` one which generates it from an `Error` and the other from an `xpc_object_t`.
|
@amomchilov for your review |
| if let error = error as? XPCError { | ||
| return error | ||
| } else if let error = error as? DecodingError { | ||
| return .decodingError(String(describing: error)) | ||
| } else if let error = error as? EncodingError { | ||
| return .encodingError(String(describing: error)) | ||
| } else if expectingOtherError { | ||
| return .other(String(describing: error)) | ||
| } else { | ||
| return .unknown | ||
| } |
There was a problem hiding this comment.
Another case of being more idiomatic with a switch:
| if let error = error as? XPCError { | |
| return error | |
| } else if let error = error as? DecodingError { | |
| return .decodingError(String(describing: error)) | |
| } else if let error = error as? EncodingError { | |
| return .encodingError(String(describing: error)) | |
| } else if expectingOtherError { | |
| return .other(String(describing: error)) | |
| } else { | |
| return .unknown | |
| } | |
| switch error { | |
| case let error as? XPCError: return error | |
| case let error as? DecodingError: return .decodingError(String(describing: error)) | |
| case let error as? EncodingError: return .encodingError(String(describing: error)) | |
| case _ where expectingOtherError: return .other(String(describing: error)) | |
| case _: return .unknown | |
| } |
There was a problem hiding this comment.
Huh, why is this more idiomatic? (I mean this genuinely, not rhetorically.) The last two cases seem rather oddly constructed where a _ is needed; particularly the penultimate one case _ where expectingOtherError.
There was a problem hiding this comment.
I'll commit this as-is, but happy to revise it later
There was a problem hiding this comment.
It's just a general trend in Swift code to prefer switch over if/else, because it makes it more clear that the predicates are all related (in that they're checking against the same operand).
A preference one way or the other isn't explicitly documented in any of the style guides I know, but some rationale is explained well here (in the context of C++, where their switch statements are waaaaaaay less powerful): https://youtu.be/raB_289NxBk?t=287
Granted, the last 2 cases here muddy the water a bit, though nonetheless I think they communicate intent very well
| internal static func fromXPCObject(_ object: xpc_object_t) -> XPCError { | ||
| if xpc_equal(object, XPC_ERROR_CONNECTION_INVALID) { | ||
| return .connectionInvalid | ||
| } else if xpc_equal(object, XPC_ERROR_CONNECTION_INTERRUPTED) { | ||
| return .connectionInterrupted | ||
| } else if xpc_equal(object, XPC_ERROR_TERMINATION_IMMINENT) { | ||
| return .terminationImminent | ||
| } else { | ||
| return .unknown | ||
| } | ||
| } |
There was a problem hiding this comment.
We do a fair bit of if/else checking with xpc_equal, do you think it's worth introducing a matcher that implements ~=, so we can do something like this? https://stackoverflow.com/a/54487077/3141234
There was a problem hiding this comment.
After this consolidation there are 7 total calls to xpc_equal in the codebase, of which this is 3 of them. So no?
There was a problem hiding this comment.
Ah, that's nice. I remember there being more of them, I guess this fixed a lot of them.
Adds two static functions to
XPCErrorone which generates it from anErrorand the other from anxpc_object_t.