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

Add exists command support #152

Merged
merged 2 commits into from Oct 6, 2019

Conversation

alirezameskin
Copy link
Contributor

closes #151

We can split all key-related commands (del, expire, exists, etc.) to separate trait.

@@ -26,6 +26,7 @@ trait StringCommands[F[_], K, V]
with Increment[F, K, V]
with Bits[F, K, V] {
def del(key: K*): F[Unit]
def exists(key: K*): F[Long]
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better if we return F[Boolean] since Redis would return either 1 if the key exists or 0 if it doesn't, and that can be easily matched to true and false.

On the other hand, I don't really like what Redis does with accepting multiple keys as input. It'll just return the sum of the partial results. For example:

redis> SET key1 "Hello"
"OK"
redis> EXISTS key1
(integer) 1
redis> EXISTS nosuchkey
(integer) 0
redis> SET key2 "World"
"OK"
redis> EXISTS key1 key2 nosuchkey
(integer) 2

I think we could do better and return true if ALL the keys exist and false if at least one key doesn't exist. Similar to what the Monoid[All] instance would do.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

@gvolpe
Copy link
Member

gvolpe commented Oct 5, 2019

We can split all key-related commands (del, expire, exists, etc.) to separate trait.

Yes, we could do that in another PR :)

@alirezameskin
Copy link
Contributor Author

We can split all key-related commands (del, expire, exists, etc.) to separate trait.

Yes, we could do that in another PR :)

I can work on it if you agree

Copy link
Member

@gvolpe gvolpe left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@gvolpe
Copy link
Member

gvolpe commented Oct 6, 2019

I can work on it if you agree

Sure @alirezameskin ! I was trying to follow how the commands are grouped in the Redis site. For example, here are all the String commands: https://redis.io/commands#string

But as you can see, it doesn't always make sense. e.g. INCR and DECR are part of it.

Feel free to propose what you have in mind and we can discuss it :)

@gvolpe gvolpe merged commit d76cdd2 into profunktor:master Oct 6, 2019
@alirezameskin alirezameskin deleted the feature/exists-command branch October 6, 2019 13:06
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.

Request for exists command support
2 participants