Skip to content

Conversation

@Avital-Fine
Copy link
Contributor

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

This looks great - a few doc tweaks and good to go 👍 - since it's just docs I'll merge in and watch builds. Need to add release notes for this one yet!

@NickCraver
Copy link
Collaborator

I'll try and tweak this a bit - added a not-exist case locally and that'll throw because it's nil coming back from Redis. We can either make that 0, or long? - checking other methods.

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Looking good - thanks again! 👍

@NickCraver
Copy link
Collaborator

@Avital-Fine I changed the return here given what redis returns and consistency with a few other APIs for that case (null does clearly indicate missing) - can you confirm this makes sense to you? If not we can tweak further.

@Avital-Fine
Copy link
Contributor Author

Avital-Fine commented Apr 13, 2022

@Avital-Fine I changed the return here given what redis returns and consistency with a few other APIs for that case (null does clearly indicate missing) - can you confirm this makes sense to you? If not we can tweak further.

@NickCraver Makes sense, but need to update the doc ;)

return null if the key does not exist
return null if the the key does not exist
revert mistake
@Avital-Fine
Copy link
Contributor Author

This looks great - a few doc tweaks and good to go +1 - since it's just docs I'll merge in and watch builds. Need to add release notes for this one yet!

@NickCraver Sorry! Always forget the release notes 😅

@NickCraver
Copy link
Collaborator

@Avital-Fine I changed the return here given what redis returns and consistency with a few other APIs for that case (null does clearly indicate missing) - can you confirm this makes sense to you? If not we can tweak further.

@NickCraver Makes sense, but need to update the doc ;)

Good call - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants