search

NFIBrokerage / spear

A sharp EventStoreDB v20+ client backed by Mint :yum:
https://hex.pm/packages/spear
Apache License 2.0
85 stars 14 forks source link

Remove the stop nack option #85

Closed dvic closed 1 year ago

dvic commented 1 year ago

Regarding

https://github.com/NFIBrokerage/spear/blob/90f7c2354426e0164eea7e64777ef9da5806e69e/lib/spear/persistent_subscription.ex#L29-L32

I think we can get rid of this option? see https://github.com/EventStore/EventStore/issues/3219#issuecomment-1003949027

the-mikedavis commented 1 year ago

Ah I remember being quite confused by :stop, thanks for the link to the thread upstream! I agree: let's remove that option from the docs and typespec. It looks like :stop/:Stop is still a valid option in the protobuf definition (https://github.com/EventStore/EventStore/blob/d6c2a0e7934279777a0cfdb126c08e4ef433b7f8/src/Protos/Grpc/persistent.proto#L59) so as long as the dialyzer doesn't reject it, we could keep around any internal code that references :stop/:Stop, for example map_nack_option in Spear.PersistentSubscription.

Would you like to submit a PR?

dvic commented 1 year ago

Ah I remember being quite confused by :stop, thanks for the link to the thread upstream! I agree: let's remove that option from the docs and typespec. It looks like :stop/:Stop is still a valid option in the protobuf definition (https://github.com/EventStore/EventStore/blob/d6c2a0e7934279777a0cfdb126c08e4ef433b7f8/src/Protos/Grpc/persistent.proto#L59) so as long as the dialyzer doesn't reject it, we could keep around any internal code that references :stop/:Stop, for example map_nack_option in Spear.PersistentSubscription.

Would you like to submit a PR?

Sure 👍