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

Expose more Command create arguments #339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liuchong
Copy link
Contributor

Motivation:

Expose more Command create arguments to let user create custom command.

Or further, do you think we should a this? Please:

  static Command create(String command, int arity, Boolean readOnly, boolean pubsub, boolean needGetKeys, KeyLocator... keyLocators) {
    return new CommandImpl(command, arity, readOnly, pubsub, needGetKeys, keyLocators);
  }

In this case, maybe we should change package io.vertx.redis.client.impl.keys to io.vertx.redis.client.keys.

@pmlopes
Copy link
Member

pmlopes commented May 27, 2022

The key locator was previously and internal implementation detail. It was not correct according to redis. It now follows redis and still it's an internal implementation detail.

Which use case are you trying to do that would require this to be public?

@liuchong
Copy link
Contributor Author

Ok, thanks for the explanation about the key locator. But how about the other arguments, the command, arity, readOnly and others...?
Before the commit 8d45692 in #328, I implemented something like that for the RedisBloom commands, use the function Command.create, only the bloom commands so I did not make a pull request here.
Although I don't need it now, because the commands are build-in supported now. But as we know there are many modules and one can also create new modules. Will the other arguments need to be public when we create new command with vertx-redis-client?

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

Successfully merging this pull request may close these issues.

2 participants