-
Notifications
You must be signed in to change notification settings - Fork 36
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
Added a function to list available buckets #153
Conversation
Thanks @lsxliron for opening this PR! I have a couple of stylistic suggestions here, but no concerns about the functionality or general approach. Would you prefer if i just merged this and just made a few of my own tweaks before releasing, or add my comments inline on the PR and let you make the changes? |
Sure thing @mmmries. Please feel free to share your suggestions. I can make changes if needed. |
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 also noticed that the main branch of elixir is showing a bunch of compiler warnings and the latest version of nats server is breaking on some of our unit tests. I'll open a separate PR to address those items, so don't worry about those CI checks here.
lib/gnat/jetstream/api/kv.ex
Outdated
""" | ||
@spec list_buckets(conn :: Gnat.t()) :: {:error, term()} | {:ok, list(String.t())} | ||
def list_buckets(conn) do | ||
case Stream.list(conn) do |
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 would swap this case
statement for a with
since it has an obvious happy-path pattern.
with {:ok, %{streams: streams}} <- Stream.list(conn) do
lib/gnat/jetstream/api/kv.ex
Outdated
case Stream.list(conn) do | ||
{:ok, %{streams: streams}} -> | ||
{:ok, | ||
streams |
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 would usually build a stream_names = Enum.flat_map(...)
on it's own before assembling the {:ok, stream_names}
tuple. That avoids extra indentation and is easier to read the flow of the function. This is a subjective opinion for sure, but most of the codebase in this project follows this pattern, so I think it's worth applying here.
lib/gnat/jetstream/api/stream.ex
Outdated
* `stream_name` - the stream name to test | ||
""" | ||
@spec is_kv_bucket_stream(stream_name :: binary()) :: boolean() | ||
def is_kv_bucket_stream(stream_name) do |
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 would suggest moving this function to the KV
module. So far I've been keeping all of the KV-specific knowledge in that module and similarly keeping everything related to Object
in its own module, so the stream module really just has knowledge of general streams.
I'm definitely open to feedback here if you had a specific reason for putting it into the stream module.
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.
No specific reason other than the fact that it's dealing with the actual stream.
Can definitely be moved to the KV module
Thanks for the great contribution and fast changes @lsxliron ❤️💙💜💚💛🧡🤎🖤 I'm going to try to sort out why we're getting different errors on some CI runs with more recent versions of Elixir/NATS and if I can sort that out within a day or two, I'll include any of those fixes in the next release along with your change |
Addresses #152