-
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 #5
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 | ||
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, | ||
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: Simplify the switch statement in the statusCode property Consider grouping the cases that return the same value to reduce repetition and improve readability. For example, you could use a single case with a pattern that matches all relevant error codes. |
||
.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 | ||
} | ||
} | ||
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: Extract repetitive code in localizedDescription into a separate method The switch statements for attachment and detachment failures contain similar code. Consider extracting this logic into a separate method to reduce duplication and improve maintainability. |
||
|
||
/// 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 language-specific syntax highlighting to code blocks
Using syntax highlighting for Swift code blocks (
swift instead of just
) could improve readability and make the documentation more polished.