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

hgetall returns a hash #77

Closed
wants to merge 1 commit into from
Closed

hgetall returns a hash #77

wants to merge 1 commit into from

Conversation

aemadrid
Copy link

Now hgetall returns a Hash(String => String) instead of an array of keys/values

@stefanwille
Copy link
Owner

Thank you.

This deals with the case of hgetall, which seems to be the one that annoys people the most. However, there are other spots where the API uses arrays but the expectation would be a hash. I'd like to see all of them changed, not one place only. Also I think this doesn't work with futures. That's where things get difficult and what kept me from doing this myself so far.

@aemadrid
Copy link
Author

I could work on adding more places where in ruby we do hashify but I'm quite clear how the use case for futures is here in Crystal. Can you tell me more about that?

@aemadrid
Copy link
Author

@stefanwille BTW I've been working on adding support for the stream commands. I'm wondering if some of these issues will be there too. Please check it out when you have a minute: https://github.com/aemadrid/crystal-redis/tree/feature/streams

@aemadrid
Copy link
Author

@stefanwille I'm guessing you are talking about futures like here: https://github.com/stefanwille/crystal-redis-examples/blob/d0dd8809f9b2cafc7f4f8f535476328c542ab712/src/pipelining.cr

I'll have to investigate how that works internally and see what could be done to make it work.

@aemadrid
Copy link
Author

@stefanwille with regards to streams here is the PR: #78

Still need to investigate what it will take to get futures/pipeling working but I'm hoping it is a good enough start.

@kostya kostya closed this in c790c9a Jul 5, 2021
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