nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.83k stars 304 forks source link

Support arrays in custom format #36

Closed chx closed 12 years ago

chx commented 12 years ago

This is mostly a copypaste of the raw array function.

nicolasff commented 12 years ago

Hi,

Thanks for starting to work on this. The main issue I see with your implementation is that list of items that contain commas would break this structure. Should they be escaped? And how about binary data?

I won't be able to work on this very soon as I'll be away from a computer for the next few days. In the meantime I'll see if I can come up with a better solution.

chx commented 12 years ago

Doesn't the raw format break with strings containing line breaks? But, sure, changing every comma to a double comma is doable. Something like http://www.planet-source-code.com/vb/scripts/ShowCode.asp?txtCodeId=10890&lngWId=3 perhaps? I am not a C coder, plainly :)

chx commented 12 years ago

In general, currently webdis emits a helpful nothing for arrays. Having something even if it's slightly limited in usage is, in my opinion, useful :) . You might know that your data doesn't contain commas. Or they are integers. For binary, I would say "no support" is just fine, use raw?

nicolasff commented 12 years ago

I agree that no reply is pretty bad. The raw format sends the data length first, so that the reader can know how many bytes to expect. This means that binary data can be sent safely.

I just want to make sure that we don't get stuck with a quirky implementation, but I agree with you that a comma-separated list is probably a good way of representing this data. If people want binary safety, they can use the raw or bson formats.

chx commented 12 years ago

Note that I left a note on the so please check it before merging (if you merge).

chx commented 12 years ago

Do we want perhaps a GET argument for separator? In my case, nothing is the best but I could live with whitespace.

nicolasff commented 12 years ago

That's a good idea, probably with an empty separator by default. Looking into it now.

nicolasff commented 12 years ago

Merged, added ?sep=... to pick a custom separator; it's the empty string by default. I also updated the unit tests and the README to keep track of this feature.

Thanks a lot, and sorry it took so long!

chx commented 12 years ago

Thanks, did you look at https://github.com/chx/webdis/commit/4732d4f3b0b5c6b61cd1456870cc6008299dd3ff#L2R90 this comment?

nicolasff commented 12 years ago

Yes, this is the code now:

    case REDIS_REPLY_STRING:
        if(sep_len && i != 0)
            *sz += sep_len;
        *sz += e->len;

(Add a separator before the string, except for the first one). The content-length is as expected.

chx commented 12 years ago

Thanks for your hard work. In case you wonder, I am using this for http://drupal.org/project/redis_ssi

nicolasff commented 12 years ago

Very cool. It's always interesting to see what people come up with :)