Skip to content

Conversation

saurik
Copy link

@saurik saurik commented Dec 21, 2012

When using pub/sub, return_buffers is ignored and channel names are forced to be strings, making it impossible to use binary channel names when accessing redis using this library.
It turns out the .toString() on these names is superfluous as the underlying parser is already returning these correctly as either strings or node.js Buffer objects based on the return_buffer setting.

(I am fully willing to believe, however, that a more complex code change is needed to work with different parser backends, or in more circumstances; I know very little about this codebase in general.)

Ready to merge checklist

  • test(s) in test.js
  • tests will fail without the PR, but succeed once applied
  • docs, if applicable
  • merges cleanly
  • coding style (4-space indents, looks similar to other code)

When using pub/sub, return_buffers is ignored and channel names are forced to be strings, making it impossible to use binary channel names when accessing redis using this library.
It turns out the .toString() on these names is superfluous as the underlying parser is already returning these correctly as either strings or node.js Buffer objects based on the return_buffer setting.
@DTrejo
Copy link
Contributor

DTrejo commented Feb 18, 2013

Would you mind adding tests for this?

Thank you,
D

@saurik
Copy link
Author

saurik commented Mar 13, 2013

I am sorry, but I was not even expecting you to consider using my patch: I only provided it as a demonstration of a change that woul fix the problem that happened to work for me. This I usually what J ask of people contributing to my open source projects: it isn't like I woul use the patch as-is anyway. In fact, I explicitly stated that I knew very little about the codebase, so I should not be the one to develop a general fix for this issue. (You especially don't want me attempting to make unit tests, as I am one of those people that believe them to be wastes of time ;P.)

BridgeAR pushed a commit that referenced this pull request Sep 21, 2015
@BridgeAR
Copy link
Contributor

Closed by 55e4a9b

@BridgeAR BridgeAR closed this Sep 21, 2015
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.

3 participants