Skip to content
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

stream interface + satisfy consumer interface #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elee1766
Copy link
Contributor

@elee1766 elee1766 commented Jan 15, 2024

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?

@dranikpg
Copy link
Owner

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?

@elee1766
Copy link
Contributor Author

elee1766 commented Feb 2, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants