-
Notifications
You must be signed in to change notification settings - Fork 18
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
Update Errors in Mqtt Client #268
Conversation
@@ -23,6 +23,20 @@ public struct CRTError: Equatable { | |||
self.name = String(cString: aws_error_name(self.code)) | |||
} | |||
|
|||
public init<T: BinaryInteger>(code: T, context: 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.
Do we still need the context string if we are generating errors from crt swift?
In the areas that are currently using context, it seems like error logging would be more appropriate. Unless we are not going to generate logs from aws-crt-swift.
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 eventually decide to keep the context, as the logging for Swift is not there yet.
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.
Unsure whether we need the context string in CommonRuntimeError. That can be changed later if we decide against it. Looks solid otherwise.
On a side note, once we do this, we need to update common to reflect the error range that aws-crt-swift is taking. (ah, you've already done it =P)
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.
Added comments. I am willing to agree to disagree here. I still feel that this PR doesn't have a strong enough use case for Swift errors. The added complexity might not justify the benefits, which seem limited to a slightly better error name that might not be significant for users.
This reverts commit 6f566ec.
Issue #, if available:
Description of changes:
unwrap
function to unwrap OptionalBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.