-
Notifications
You must be signed in to change notification settings - Fork 5
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
Change NatsSubscription to a throwing AsyncSequence #79
Conversation
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
This might be good for checking - both ways actually @piotrpio |
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@Jarema thanks. I added handling queue in permissions denied error (if present) and fixed |
Great. Ready for review? :) |
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.
Just one change request.
@@ -33,19 +57,77 @@ public enum NatsError { | |||
} | |||
init(_ error: String) { | |||
let normalizedError = error.trimWhitespacesAndApostrophes().lowercased() |
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.
we can't normalize whole error, as it can contain case-sensitive subject that publish failed to.
Look here: nats-io/nats.rs#1258
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.
Right, for permission violation that's not a problem here as we only use the normalized version of the error to compare to "permissions violation"
. I changed the .proto
case to include raw error string.
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!
Signed-off-by: Piotr Piotrowski piotr@synadia.com