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

fix: subscribe should be of type string #1229

Merged
merged 1 commit into from
Dec 26, 2019
Merged

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Dec 24, 2019

arguments for SUBSCRIBE were in the wrong format, which caused this bug: mmkal/handy-redis#46

this change makes them match unsubscribe: https://github.com/antirez/redis-doc/blob/c882f1d3a767a096453489b00d7ec5bcc2eceaa7/commands.json#L2997-L3010

arguments for SUBSCRIBE were in the wrong format, which caused this bug: mmkal/handy-redis#46

this change makes them match unsubscribe: https://github.com/antirez/redis-doc/blob/c882f1d3a767a096453489b00d7ec5bcc2eceaa7/commands.json#L2997-L3010
@itamarhaber
Copy link
Member

Hello @mmkal

I understand the PR and accepting it will not be a problem in my opinion. That said, other commands have similar definitions, although the arrays' lengths are > 1 - how does you client handle that?

@mmkal
Copy link
Contributor Author

mmkal commented Dec 25, 2019

Hi @itamarhaber, thanks for the reply. I'm curious which other commands you're referring to? The client parses arguments looking like ["key", "key"] as tuples of length two, so in typescript an array like ['foo', 'bar'] is expected, and that's fine. It was just this one case that was demanding a "tuple" of length one, which doesn't make sense. If there are other arguments with the same problem I will happily also edit them as a follow-up. Even so, the handy-redis client linked flattens all arguments so there is an ugly workaround right now.

@itamarhaber
Copy link
Member

This one's the only one with len == 1, but BITFIELD and HSET have such ones with len == 2.

@mmkal
Copy link
Contributor Author

mmkal commented Dec 26, 2019

BITFIELD is working correctly: https://github.com/mmkal/handy-redis/blob/6450195575996c7b1896c569d3bf37b6c5326002/src/generated/interface.ts#L300-L303, since the value in command.json corresponds to the documentation, so the types are used like client.bitfield('mykey', ['GET', 'u4', 0]).

As for HSET - the types the client is currently using are out of date, but should work fine too from looking at the latest docs. If this PR can get merged reasonably quickly, I'll wait for that to update to avoid churn.

@itamarhaber itamarhaber merged commit 6fb67f0 into redis:master Dec 26, 2019
@mmkal mmkal deleted the patch-1 branch January 3, 2020 03:38
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.

2 participants