gtrs icon indicating copy to clipboard operation
gtrs copied to clipboard

stream interface + satisfy consumer interface

Open elee1766 opened this issue 1 year ago • 2 comments

a few breaking changes here....

  1. people who were using type *Stream[T] before should change to either Stream[T] or *RedisStream[T]
  2. the two consumers now both properly implement the consumer interface, at the cost of making Close() return any and requiring the user to cast the type out.

@dranikpg did you know that neither consumer matched the consumer interface? i noticed when i added the interface check. im not sure if my any solution here is good, it was just easy.

perhaps its better to unite these two return types into a single return type that can be shared by the close method? not sure. any thoughts?

elee1766 avatar Jan 15 '24 05:01 elee1766

perhaps its better to unite these two return types into a single return type that can be shared by the close method? not sure. any thoughts?

Good question. On one hand, it seems useful to be able to close the stream with the interface handle, on the other it's a little weird what return type to choose for missed info

We could use []InnerAck in both places (instead of StreamIds for the regular one), but it's a little confusing and inconvenient (for users that don't use the interface and I assume that's all of them currently 😆)

Yet, if we don't include the Close() function the Consumer interface makes little sense, as it's identical to just a <-chan 🤔

A third option is having another function like CloseSilent() which

  • Makes the interface more versatile than just a <- chan
  • Allows not sacrificing on typing in the implementations themselves

But still is weird 😢 Wdyt about this last option?

dranikpg avatar Jan 20 '24 14:01 dranikpg

perhaps its better to unite these two return types into a single return type that can be shared by the close method? not sure. any thoughts?

Good question. On one hand, it seems useful to be able to close the stream with the interface handle, on the other it's a little weird what return type to choose for missed info

We could use []InnerAck in both places (instead of StreamIds for the regular one), but it's a little confusing and inconvenient (for users that don't use the interface and I assume that's all of them currently 😆)

Yet, if we don't include the Close() function the Consumer interface makes little sense, as it's identical to just a <-chan 🤔

A third option is having another function like CloseSilent() which

* Makes the interface more versatile than just a `<- chan`

* Allows not sacrificing on typing in the implementations themselves

But still is weird 😢 Wdyt about this last option?

hey sorry, have been busy.

i think there is some merit in making a function Close() error which statisfies io.Closer, and that is the CloseSilent

and add say a CloseVerbose to the existing methods for advanced users, and they can type assert it out if they need it. that function could return (anything, error)

it's a breaking change, but it's easy to fix and I think most users won't be affected.

users upgrading will already have to decide if they wish to upgrade to the interface, and they will have to change the type if they wish to access the special close method anyways, so i think it's good to use this as an opportunity to be more standard with io.Closer

the final secret wildcard option is to add a second generic type parameter that dictates the response type of close :)

what do you think?

elee1766 avatar Feb 02 '24 03:02 elee1766