Add Streams support #799
Add Streams support #799
Conversation
| ref: redis/redis-py#1040 |
byroot left a comment
byroot left a comment There was a problem hiding this comment.
Just some minor nitpicky comments.
The code looks good, but I need to take some time to think about the API.
lib/redis.rb Outdated
| HashifyStreamEntries = | ||
| lambda { |reply| | ||
| reply.map do |entry_id, values| | ||
| [entry_id, Hash[values.each_slice(2).to_a]] |
There was a problem hiding this comment.
values.each_slice(2).to_h is simpler.
lib/redis.rb Outdated
| synchronize { |client| client.call([:xlen, key]) } | ||
| end | ||
| | ||
| # Fetches entries from one or multiple streams. You're able to use blocking if needed. |
There was a problem hiding this comment.
You're able to use blocking if needed. I think this needs to be phrased differently.
Maybe Optionally blocking ?
lib/redis.rb Outdated
| synchronize { |client| client.call(args, &HashifyStreamEntries) } | ||
| end | ||
| | ||
| # Fetches entries of the stream ordered by descending. |
There was a problem hiding this comment.
ordered by descending. -> in descending order.
supercaracal force-pushed the add-streams-support branch 4 times, most recently from 2f9bdff to 5a9305b Compare | I have fixed the following additional things.
|
supercaracal force-pushed the add-streams-support branch from 5a9305b to d1fc118 Compare | I have fixed as the following.
|
supercaracal force-pushed the add-streams-support branch 4 times, most recently from 53105c4 to c17db7f Compare 
| end | ||
| end | ||
| | ||
| def omit_version(min_ver) |
There was a problem hiding this comment.
This isn't used anywhere. Can you remove it from the PR please?
There was a problem hiding this comment.
The above helper method is used by the following linter.
https://github.com/redis/redis-rb/pull/799/files#diff-a14ac8847c163e36437c11a9aa3c7643R8-R11
test/lint/streams.rb
Lint::Streams#setup
There was a problem hiding this comment.
Weird, that's a broken link for me and ctrl + F omit_version doesn't match anything.
There was a problem hiding this comment.
Argh, nvm, GitHub chose to hide that part of the diff.
There was a problem hiding this comment.
I'm sorry.
def setup super omit_version(MIN_REDIS_VERSION) end
| I think this looks very good. It just need a rebase and some minor ajustement. I'd like to merge it and play with it a bit, and eventually release a 4.1.0 with this & the cluster support. |
| Can you rebase ? I'll merge afterward. |
supercaracal force-pushed the add-streams-support branch from c17db7f to 3a16a0e Compare | I have rebased. I appreciate for your review. |
2 participants
I'd like to use Redis Streams.