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

support full set of redis hash commands #20 #21

Merged
merged 1 commit into from
Aug 8, 2024
Merged

support full set of redis hash commands #20 #21

merged 1 commit into from
Aug 8, 2024

Conversation

72squared
Copy link
Collaborator

implemented all the hash commands.

I did not support streaming with hscan, we can leave that for a future implementation. Want to use reactive streams pattern.

@72squared
Copy link
Collaborator Author

I think I uncovered a Lettuce bug while testing. If you call hrandfieldWithvalues(K key, long count) on a key with fewer elements in it than the count, there is a nasty low-level redis parse error. I changed my test to avoid that case, but we might want to report it as an issue.

@72squared 72squared self-assigned this Aug 8, 2024
@72squared
Copy link
Collaborator Author

converted all the responses to List instead of Seq. I also changed some of the inputs to List but perhaps that is a step too far? Accepting Seq or Iterable as an input is probably acceptable. I could change those input changes back to Seq or Iterable even?

https://en.wikipedia.org/wiki/Robustness_principle

Comment on lines 69 to 83
/**
* Get the values of multiple hash fields
* @param key The key
* @param fields The fields
* @return A map of field -> value for each field that exists or None if the field does not exist
*/
def hMGet(key: K, fields: K*): Future[ListMap[K, Option[V]]]

/**
* Get the values of multiple hash fields
* @param key The key
* @param fields The fields
* @return A map of field -> value for each field that exists or None if the field does not exist
*/
def hMGet(key: K, fields: => List[K]): Future[Map[K, Option[V]]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep both methods since they seem to achieve the same operation ?

Could we do maybe def hMGet(key: K, fields: K*): Future[Map[K, Option[V]]] only so that it's more in line with other operations ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally hate the fields: K* approach since it prevents adding additional parameters later. Since that is unlikely to happen though, yeah I can get rid of the def hMGet(key: K, fields: => List[K]) signature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I removed the def hMGet(key: K, fields: => List[K]) signature and left the other one. That should resolve the open issues.

Copy link
Owner

@scoquelin scoquelin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing hard on the hash commands! 🚀

@72squared 72squared merged commit c6b6572 into main Aug 8, 2024
1 check passed
@72squared 72squared deleted the jl-hash branch August 8, 2024 19:07
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