search

stefanwille / crystal-redis

Full featured Redis client for Crystal
MIT License
380 stars 61 forks source link

Feature/streams #78

Closed aemadrid closed 1 year ago

aemadrid commented 5 years ago

Adds support for all stream related commands in value mode. Mostly porting the code from the ruby driver.

Note: Still need to investigate how to support futures for pipelining.

larskluge commented 5 years ago

I tried the very basics and it's looking good so far. All as expected/known from its Ruby counterpart.

And built a tiny first project based on it: https://github.com/larskluge/rsone

Will report if I find anything down the road.

Thanks for your work @aemadrid !

aemadrid commented 5 years ago

@larskluge thx! I've looked at implementing 1hashify1/etc for pipelines/futures but I cannot seem to get it to work. Not sure how you feel about having streams not working there.

stefanwille commented 5 years ago

@aemadrid thanks for the PR. This looks like good start.

If you can get away with using the builtin commands primitives such as string_command(), there is a good chance that your implementation works with pipelines/futures too.

I see the spec fails in CI. Is it green for you in local development?

aemadrid commented 5 years ago

@stefanwille it does pass locally. I think the problem is that travis doesn't have the right redis version (>= 5). BTW I figured out a solution to the pipeline issue but it will be a much bigger PR. My idea is to command(args, unwrapper) where unwrapper is a strategy like :int_or_nil, :string_array, etc. so that could be applied at the right time. Very similar to what the ruby driver does. That should work for pipelines, transactions, etc. and also work for all hash, stream, etc. responses.

Xosmond commented 5 years ago

@aemadrid @stefanwille Is there any news on this PR?

stefanwille commented 5 years ago

No

anykeyh commented 1 year ago

Hello,

I was planning to work precisely on this feature for a project of mine. Very happy that someone already started it <3.

However, what is currently blocking this from being merged to master? Except for the travis Redis version, is there anything needed to be implemented?

I would be happy to help.

kostya commented 1 year ago

I think it mergable, but need update to master (btw method hashify was already added). Then if specs passing, also check that this feature works in real.