-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add connection events #32
Conversation
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
d86dae3
to
65addc2
Compare
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.
This looks really good.
Few comments.
@discardableResult | ||
public func on(_ events: [NatsEventKind], _ handler: @escaping (NatsEvent) -> Void) -> String { | ||
guard let connectionHandler = self.connectionHandler else { | ||
return "" |
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.
what does returning this mean?
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.
Nothing really, the alternative would be to throw an error here and I thought it might be better to avoid it... Normally this method returns a callback ID so that it can be closed with off()
.
@@ -362,7 +381,7 @@ class ConnectionHandler: ChannelInboundHandler { | |||
|
|||
func write(operation: ClientOp) throws { | |||
guard let allocator = self.channel?.allocator else { | |||
throw NSError(domain: "nats_swift", code: 1, userInfo: ["message": "no allocator"]) | |||
throw NatsClientError("no allocator") |
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.
maybe we should call those with a prefix like "internal error" to hint that this is not a user or server fault?
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 added prefixes on several errors
case connected = "connected" | ||
case disconnected = "disconnected" | ||
case closed = "closed" | ||
case error = "error" |
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.
should this variant also ontain the error?
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.
This is just an event kind so that it can be easily registered. I went with 2 types - NatsEvent
and NatsEventKind
because NatsEvent
may contain additional data for an event (like in the case of .error
), but the kind is just a simple string for registering.
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.
ah, I see it now.
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
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.
LGTM!
case connected = "connected" | ||
case disconnected = "disconnected" | ||
case closed = "closed" | ||
case error = "error" |
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.
ah, I see it now.
No description provided.