-
Notifications
You must be signed in to change notification settings - Fork 990
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
chore: split geo and zset families #4421
Conversation
src/server/geo_family.h
Outdated
class GeoFamily { | ||
public: | ||
static void Register(CommandRegistry* registry); | ||
using SinkReplyBuilder = facade::SinkReplyBuilder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think you need it here, nor its declaration at line 12.
using SinkReplyBuilder = facade::SinkReplyBuilder; | ||
template <typename T> using OpResult = facade::OpResult<T>; | ||
|
||
static void ZAddGeneric(std::string_view key, const ZParams& zparams, ScoredMemberSpan memb_sp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for exposing ZAddGeneric here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i see.
please add a comment that it's being used in geo_family.cc code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Signed-off-by: kostas <[email protected]>
I was longing for this change for some time now and I grabbed the opportunity since I already worked on #4420
I am not sure how I could reduce the size of the PR.
There are no functional changes only a refactor.
TODO for separate PR
ZSetFamily::
to a separate file