feat: Split sortedSetFetch up into ByRank and ByScore functions #280
Conversation
As per the API design discussion and the pattern we established in the node.js SDK, this commit splits the SortedSetFetch into two functions, one for fetching by score and one for by rank. This prevents the user from needing to reason about which combinations of arguments are and are not legal together.
| StartRank *int32 | ||
| EndRank *int32 |
There was a problem hiding this comment.
Sorry maybe this has been discussed somewhere, but why do we use pointers for StartRank and EndRank?
There was a problem hiding this comment.
I think it's because we want to be able to use nil to express that we aren't specifying them.
There was a problem hiding this comment.
I know that that's how we did for SortedSetFetchByRank and SortedSetFetchByScore types, but I'm not sure why these were pointers.
There was a problem hiding this comment.
if they were not pointers, how would I express that I want to use the defaults?
You may be right that there is another way to do it! But that was my understanding.
There was a problem hiding this comment.
Oh I see. It makes sense pointers were used so that they can be nil since these can be "optional."
I think it makes sense for those values to be optional since ByScore and ByRank were a part of the fetch API's params.
But now we're breaking into two separate APIs specifically named ByScore and ByRank, it feels a bit weird for those values to be optional?
What do you think? @cprice404
There was a problem hiding this comment.
But it looks like it's already decided in the API doc.
There was a problem hiding this comment.
They are optional because you could pass either one of them or the other, or both.
For example, you could pass only EndRank of 10 if you wanted to get the first 10. Or you could pass only StartRank of 10 and then get all of the elements after the 10th one.
Also note that these numeric fields were pointers even inside of the ByRank/ByScore objects before.
There was a problem hiding this comment.
It makes sense! Thank you for the clarification.
Yup, I saw that after leaving the above comment and figured that this is something decided with reasons.
| MinScore *float64 | ||
| MaxScore *float64 | ||
| Offset *uint32 | ||
| Count *uint32 |
There was a problem hiding this comment.
Same comment as above.
Why do we use pointers here?
* feat: Split sortedSetFetch up into ByRank and ByScore functions As per the API design discussion and the pattern we established in the node.js SDK, this commit splits the SortedSetFetch into two functions, one for fetching by score and one for by rank. This prevents the user from needing to reason about which combinations of arguments are and are not legal together.
As per the API design discussion and the pattern we established in the node.js SDK, this commit splits the SortedSetFetch into two functions, one for fetching by score and one for by rank. This prevents the user from needing to reason about which combinations of arguments are and are not legal together.