Adding send_command_buf function sending reply as Buffers instead of Strings

[vitaliylag]

Adding send_command_buf function. The same as send_command but reply will be sent to callbacks as Buffers instead of Strings.

[BridgeAR]

The general idea is fine, but I would not add a new function, but instead add a new parameter to the already existing function and more important: add a buffer function for each command.

It won't work though as it's currently suggested. The function would have to make sure that the parser returns buffers from that point on without changing the results of any other call no matter what option is set.

And using a try catch block around send_command destroys the purpose of the parameter validation.

[vitaliylag]

And using a try catch block around send_command destroys the purpose of the parameter validation.

I don't use try catch, I use try finally.

but instead add a new parameter

Calling command not added to node_redis yet will be uncomfortably because parameters will be too difficult. If you mean internal_send_command only then it's OK (and of course wrapper function can be also added).

add a buffer function for each command

I think great idea. And delete uppercase commands. If somebody use them he can just copy them by himself. Or you just can add Redis.createUpperCaseCommands() function that will copy all commands. Actually you can also add Redis.createBufferCommands() function. If need I can implement them.

It won't work though as it's currently suggested. The function would have to make sure that the parser returns buffers from that point on without changing the results of any other call no matter what option is set.

Sorry, I didn't understand (I guess my English is bad). «It won't work» — my function (send_command_buf)? «without changing the results of any other call» — my function doesn't change other calls replies.

[BridgeAR]

I don't use try catch, I use try finally.

True indeed, that works and my comment should be ignored. One thing about the try finally though: it might be worth thinking about not having any rescue plan in case of a misuse. The only way how this would not end up in a crash is by listening for unhandled exceptions and in that case the app should at least restart and recovering is not intended.
So guarding against the misuse is a performance hit that we do not have to take.

Calling command not added to node_redis yet will be uncomfortably because parameters will be too difficult. If you mean internal_send_command only then it's OK (and of course wrapper function can be also added).

I agree that it's uncomfortable for users, so yes it should only be used for internal_send_command (using a new state can be prevented therefore).
This pull request is about having a nice API after all, as there's already the detect_buffer option that allows users to return buffers individually.

I think great idea. And delete uppercase commands. If somebody use them he can just copy them by himself. Or you just can add Redis.createUpperCaseCommands() function that will copy all commands. Actually you can also add Redis.createBufferCommands() function. If need I can implement them.

I'm +1 on removing the uppercase commands. That's already plannend, but this can only happen in the next major version. Adding the extra functions is not necessary in that case, as any user can just write an alias for the original to use it as uppercase command.

It won't work though as it's currently suggested. The function would have to make sure that the parser returns buffers from that point on without changing the results of any other call no matter what option is set.

Sorry, I didn't understand (I guess my English is bad). «It won't work» — my function (send_command_buf)? «without changing the results of any other call» — my function doesn't change other calls replies.

It's not about your function changing another call directly. But the default return value of node_redis is of type String. In that case, the parser setting has to be changed to return buffers. And then the handle_detect_buffers_reply has to be set as function for handle_reply to return strings in case of the non buffer functions. Otherwise all "normal" string functions would return buffers too.

const Redis = require('redis')
const assert = require('assert')
const client = Redis.createClient()

assert(!client.buffers) // Default is to return strings, therefore buffers is false

client.set('foo', 'bar')

// This has to trigger setting `client.buffers` to true and to reset the parser to return buffers
client.getBuffer('foo', function (err, res) {
  assert.strictEqual(res.inspect(), '<Buffer 62 61 72>')
})
// This has to keep on working even though the parser returns buffers
client.get('foo', function (err, res) {
  assert.strictEqual(res, 'bar')
})

[vitaliylag]

Features:

1. Working propertly even if detect_buffers and return_buffers was set to false.
2. Added support for OfflineCommand constructor.
3. Added "b_" prefixed commands. For example redis.b_hget("h", "a", callback);
4. Changed send_command_buf function code (see comments there).

Code changing:

1. this.buffers property is renamed to this.using_buffer_parser.
2. argument buffer_args is renamed to buffer_reply in Command constructor. It sets to true if:
   • this.options.return_buffers
   • or buffer_args && this.options.detect_buffers
   • or buffer_reply
   • (see internal_send_command function)
   • That means that we don't need to check options on reply anymore, we just need check buffer_reply only.
3. handle_detect_buffers_reply has been removed. Well, you can restore it when this.using_buffer_parser is falsy, but I think it make code more complicated.

but instead add a new parameter to the already existing function

I added parameter because it's easy for "b_" prefixed commands but send_command_buf still using this.cur_command_ret_buf because if some command overwritten then it must work without changing code (see comments in send_command_buf).

Also I think detect_buffers option should be deprecated.

[vitaliylag]

Now it supports multi and batch. Also I have added tests.

[vitaliylag]

Now it supports individual commands (including multi individual commands).

@BridgeAR, there is no tests for individual commands but I can add if you say this PR can be merged.

[BridgeAR]

I refactored passing the command_obj to be created in a earlier stage in #1074. If you rebase, this should allow to simplify some of the code.

[vitaliylag]

How to rebase?

[BridgeAR]

I just released v.2.6 and you can rebase to the master instead of the other branch.

Just write something like:

git remote add upstream https://github.com/NodeRedis/node_redis.git
git fetch upstream
git rebase -i upstream/master

You'll get all the commits from the master and can rebase your against them to solve the issues individually. This is nicer than using a merge. And when you're done you can push your changed history to this branch.

[vitaliylag]

rebased and improved tests

[vitaliylag]

@BridgeAR, what about merge?

[vitaliylag]

is the project alive?

[stockholmux]

@vitaliylag Yep. Although I haven't seen Ruben around in a while. I'm assuming this commit is no longer valid, right? It's 18 months old.

Any particular way you'd like to help out?

[vitaliylag]

I didn't see any major changes last 18 months so I guess nobody maintain the project.

I haven't seen Ruben around in a while

So the project is not alive I guess... :)

Any particular way you'd like to help out?

No. If I were offered to become second admin I would accept it to merge my pull requests without any problems (in my server I use version with promises support so it is second not published PR) but I wouldn't maintain the project.

[BridgeAR]

I am going to look into this at some point soonish. I am aware that I did not invest enough time in here but I will get to that soon again and finally get some stuff moving again.