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

Patch to support RedisTimeSeries commands. #137

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

Patch to support RedisTimeSeries commands. #137

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 23, 2019

RedisTimeSeries https://oss.redislabs.com/redistimeseries/
. character was unsupported but now it works for commands beginning with 'ts'.
Assumption made that no other commands begin with 'ts'.
usage: $redis->tscreate(....), tsadd(....) etc.

First pull request made so excuse me if I've missed something.
No tests written as I'm not sure how to do that.

Not sure the change is good enough but please review.

RedisTimeSeries https://oss.redislabs.com/redistimeseries/
. character was unsupported but now it works for command beginning with 'ts'.
Assumption made that no other commands begin with 'ts'.
usage: $redis->tscreate(....), tsadd(....) etc.
@dams
Copy link
Member

dams commented Jul 23, 2019

Hi,

Thank you for your contribution. I'm not sure adding a dot after "ts" for all command that start with it is a good long term solution. There are other Redis modules, that all have a dot in the command name. It would be better to have Redis.pm to offer a better support of these "dotted" commands.

I'll check out how other clients do

@shogo82148
Copy link
Contributor

FYI, $redis->${\"ts.create"} is one of workaround for redis modules.
but it looks bad knowhow. I want official support of Redis.pm.

in addition, there are "underlined" commands such asgeoradius_ro #128

@dams
Copy link
Member

dams commented Jul 24, 2019

Maybe it’s time to introduce an other API, like $redis->command(‘KEYS’,’*’) ?
It would become the main way to use the module, but we’d keep existing methods as shortcuts

@ghost
Copy link
Author

ghost commented Jul 24, 2019

Sounds like a viable option I think.

@shogo82148
Copy link
Contributor

It sounds good, but there is already the "command" command https://redis.io/commands/command
we should choice new method's name very carefully.

@dams
Copy link
Member

dams commented Jul 25, 2019

what would be great at this point is to check how other Perl clients but also other language's clients do. If some of you have used other clients, please share your knowledge/experience

@ghost
Copy link
Author

ghost commented Jul 25, 2019

A quick check of how clients for other languages do (from redis client list), yields that some provides a "wrapper" method like Do(..), Command(..) or Execute(..) for commands. Providing meaningful method names such as Set(..), Get(..), Incrby(..) etc. is very common. I believe that implementing a wrapper method like $redis->do(..) or similar as you suggest is the way forward. I don't see that there is any other way if we want to support commands having a dot in their names as (if memory serves) Perl does not support dot chars in sub names.

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

Successfully merging this pull request may close these issues.

None yet

2 participants