-
Notifications
You must be signed in to change notification settings - Fork 0
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
[ECO-4952] Start implementing room lifecycle spec #3
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,37 +16,163 @@ public enum ErrorCode: Int { | |
/// TODO this code is a guess, revisit in https://github.com/ably-labs/ably-chat-swift/issues/32 | ||
case inconsistentRoomOptions = 1 | ||
|
||
case messagesAttachmentFailed = 102_001 | ||
case presenceAttachmentFailed = 102_002 | ||
case reactionsAttachmentFailed = 102_003 | ||
case occupancyAttachmentFailed = 102_004 | ||
case typingAttachmentFailed = 102_005 | ||
|
||
case messagesDetachmentFailed = 102_050 | ||
case presenceDetachmentFailed = 102_051 | ||
case reactionsDetachmentFailed = 102_052 | ||
case occupancyDetachmentFailed = 102_053 | ||
Comment on lines
+19
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Consider consolidating error types for attachment and detachment failures The current approach of having separate error codes for each feature's attachment and detachment failures leads to repetitive code. Consider creating a more generic error type with associated values for the feature and operation type (attach/detach). This could simplify the ErrorCode enum and make it easier to maintain. |
||
case typingDetachmentFailed = 102_054 | ||
|
||
case roomInFailedState = 102_101 | ||
case roomIsReleasing = 102_102 | ||
case roomIsReleased = 102_103 | ||
|
||
/// The ``ARTErrorInfo.statusCode`` that should be returned for this error. | ||
internal var statusCode: Int { | ||
// TODO: These are currently a guess, revisit in https://github.com/ably-labs/ably-chat-swift/issues/32 | ||
// TODO: These are currently a guess, revisit once outstanding spec question re status codes is answered (https://github.com/ably/specification/pull/200#discussion_r1755222945), and also revisit in https://github.com/ably-labs/ably-chat-swift/issues/32 | ||
switch self { | ||
case .inconsistentRoomOptions: | ||
case .inconsistentRoomOptions, | ||
.messagesDetachmentFailed, | ||
.presenceDetachmentFailed, | ||
.reactionsDetachmentFailed, | ||
.occupancyDetachmentFailed, | ||
.typingDetachmentFailed, | ||
.roomInFailedState, | ||
.roomIsReleasing, | ||
.roomIsReleased: | ||
400 | ||
case .messagesAttachmentFailed, | ||
.presenceAttachmentFailed, | ||
.reactionsAttachmentFailed, | ||
.occupancyAttachmentFailed, | ||
.typingAttachmentFailed: | ||
// TODO: This is currently a best guess based on the limited status code information given in the spec at time of writing (i.e. CHA-RL1h4); it's not clear to me whether these error codes are always meant to have the same status code. Revisit once aforementioned spec question re status codes answered. | ||
500 | ||
} | ||
} | ||
} | ||
|
||
/** | ||
The errors thrown by the Chat SDK. | ||
|
||
This type exists in addition to ``ErrorCode`` to allow us to attach metadata which can be incorporated into the error’s `localizedDescription`. | ||
This type exists in addition to ``ErrorCode`` to allow us to attach metadata which can be incorporated into the error’s `localizedDescription` and `cause`. | ||
*/ | ||
internal enum ChatError { | ||
case inconsistentRoomOptions(requested: RoomOptions, existing: RoomOptions) | ||
case attachmentFailed(feature: RoomFeature, underlyingError: ARTErrorInfo) | ||
case detachmentFailed(feature: RoomFeature, underlyingError: ARTErrorInfo) | ||
case roomInFailedState | ||
case roomIsReleasing | ||
case roomIsReleased | ||
|
||
/// The ``ARTErrorInfo.code`` that should be returned for this error. | ||
internal var code: ErrorCode { | ||
switch self { | ||
case .inconsistentRoomOptions: | ||
.inconsistentRoomOptions | ||
case let .attachmentFailed(feature, _): | ||
switch feature { | ||
case .messages: | ||
.messagesAttachmentFailed | ||
case .occupancy: | ||
.occupancyAttachmentFailed | ||
case .presence: | ||
.presenceAttachmentFailed | ||
case .reactions: | ||
.reactionsAttachmentFailed | ||
case .typing: | ||
.typingAttachmentFailed | ||
} | ||
case let .detachmentFailed(feature, _): | ||
switch feature { | ||
case .messages: | ||
.messagesDetachmentFailed | ||
case .occupancy: | ||
.occupancyDetachmentFailed | ||
case .presence: | ||
.presenceDetachmentFailed | ||
case .reactions: | ||
.reactionsDetachmentFailed | ||
case .typing: | ||
.typingDetachmentFailed | ||
} | ||
case .roomInFailedState: | ||
.roomInFailedState | ||
case .roomIsReleasing: | ||
.roomIsReleasing | ||
case .roomIsReleased: | ||
.roomIsReleased | ||
} | ||
} | ||
|
||
/// A helper type for parameterising the construction of error messages. | ||
private enum AttachOrDetach { | ||
case attach | ||
case detach | ||
} | ||
|
||
private static func localizedDescription( | ||
forFailureOfOperation operation: AttachOrDetach, | ||
feature: RoomFeature | ||
) -> String { | ||
let featureDescription = switch feature { | ||
case .messages: | ||
"messages" | ||
case .occupancy: | ||
"occupancy" | ||
case .presence: | ||
"presence" | ||
case .reactions: | ||
"reactions" | ||
case .typing: | ||
"typing" | ||
} | ||
|
||
let operationDescription = switch operation { | ||
case .attach: | ||
"attach" | ||
case .detach: | ||
"detach" | ||
} | ||
|
||
return "The \(featureDescription) feature failed to \(operationDescription)." | ||
} | ||
|
||
/// The ``ARTErrorInfo.localizedDescription`` that should be returned for this error. | ||
internal var localizedDescription: String { | ||
switch self { | ||
case let .inconsistentRoomOptions(requested, existing): | ||
"Rooms.get(roomID:options:) was called with a different set of room options than was used on a previous call. You must first release the existing room instance using Rooms.release(roomID:). Requested options: \(requested), existing options: \(existing)" | ||
case let .attachmentFailed(feature, _): | ||
Self.localizedDescription(forFailureOfOperation: .attach, feature: feature) | ||
case let .detachmentFailed(feature, _): | ||
Self.localizedDescription(forFailureOfOperation: .detach, feature: feature) | ||
case .roomInFailedState: | ||
"Cannot perform operation because the room is in a failed state." | ||
case .roomIsReleasing: | ||
"Cannot perform operation because the room is in a releasing state." | ||
case .roomIsReleased: | ||
"Cannot perform operation because the room is in a released state." | ||
} | ||
} | ||
|
||
/// The ``ARTErrorInfo.cause`` that should be returned for this error. | ||
internal var cause: ARTErrorInfo? { | ||
switch self { | ||
case let .attachmentFailed(_, underlyingError): | ||
underlyingError | ||
case let .detachmentFailed(_, underlyingError): | ||
underlyingError | ||
case .inconsistentRoomOptions, | ||
.roomInFailedState, | ||
.roomIsReleasing, | ||
.roomIsReleased: | ||
nil | ||
} | ||
} | ||
} | ||
|
@@ -58,6 +184,11 @@ internal extension ARTErrorInfo { | |
userInfo["ARTErrorInfoStatusCode"] = chatError.code.statusCode | ||
userInfo[NSLocalizedDescriptionKey] = chatError.localizedDescription | ||
|
||
// TODO: This is kind of an implementation detail (that NSUnderlyingErrorKey is what populates `cause`); consider documenting in ably-cocoa as part of https://github.com/ably-labs/ably-chat-swift/issues/32. | ||
if let cause = chatError.cause { | ||
userInfo[NSUnderlyingErrorKey] = cause | ||
} | ||
|
||
self.init( | ||
domain: errorDomain, | ||
code: chatError.code.rawValue, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/// The features offered by a chat room. | ||
internal enum RoomFeature { | ||
case messages | ||
case presence | ||
case reactions | ||
case occupancy | ||
case typing | ||
} |
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.
suggestion (documentation): Consider adding a brief explanation of the purpose of spec tags
It might be helpful to briefly explain why these spec tags are used at the beginning of the 'Attributing tests to a spec point' section. This would provide immediate context for developers.