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

Sorted set inconsistencies. #167

Closed
5 tasks done
schwern opened this issue Feb 24, 2023 · 4 comments · Fixed by #239
Closed
5 tasks done

Sorted set inconsistencies. #167

schwern opened this issue Feb 24, 2023 · 4 comments · Fixed by #239
Assignees
Labels
Milestone

Comments

@schwern
Copy link
Contributor

schwern commented Feb 24, 2023

SortedSetGetRank, SortedSetGetScore, and SortedSetRemove all reference elements by their value. However they all take those values in different ways...

  • SortedSetGetRank takes ElementName Value
  • SortedSetGetScore takes ElementNames []Value
  • SortedSetIncrement takes ElementName Value
  • SortedSetRemove takes ElementsToRemove which is SortedSetRemoveNumElements which is a struct containing just Name (not Value).

We're still referring to the "element name" rather than "element value" in these functions.

  • SortedSetGetScore takes Values []Value
  • Rename SortedSetGetScore -> SortedSetGetScores
  • SortedSetGetRank takes Value Value
    • The request only takes a single value, because that's how Redis does it.
    • Chris says not to loop inside the method because something something concurrency.
  • SortedSetIncrement takes Value Value
  • SortedSetRemove takes Values []Value.
    • Remove the option to remove all. We can add it back later if necessary. Deleting a SortedSet can be done with client.delete(collectionName).
@schwern schwern added the enhancement New feature or request label Feb 24, 2023
@schwern schwern self-assigned this Feb 24, 2023
@schwern schwern added this to the SDK 1.0 milestone Feb 27, 2023
@pgautier404
Copy link
Contributor

@schwern In case this isn't on your radar, it looks like sorted sets are inconsistent from the other types in that their Hit responses are using public properties instead of methods to expose values. All of the other CDT store their values as private data members and expose them to users as methods.

@schwern
Copy link
Contributor Author

schwern commented Mar 6, 2023

@pgautier404 In almost (dictionaryIncrement being an exception) every other case there's some need to convert the value. SortedSets largely return numbers (score or rank). The plan was to change them to simple type conversions and avoid having the user jump through an accessor. For example, type SortedSetRank uint64.

  • SortedSetFetch* needs String and Bytes methods. ✅
  • SortedSetGetRank returns a number.
  • SortedSetGetScores returns an array of numbers.
  • SortedSetIncrement returns a number.

I could add an accessor method for consistency 🤷 but I'd like to keep the single value types.

@schwern
Copy link
Contributor Author

schwern commented Mar 6, 2023

@pgautier404 I thought about it some more.

Because we're so inexperienced with Go, I think it's best to make all the struct members private and provide an accessor. We can always change it to a simple type conversion later.

@schwern
Copy link
Contributor Author

schwern commented Mar 6, 2023

@pgautier404 ...except responses are in a different package now, so we can't access private members.

It seems silly to have both a single public numeric member, and an accessor for that public member. I'm going to keep it simple and make them simple type conversions and provide a method for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants