Newparser

[sbromberger]

This is a fix-all with a new parsing structure that takes advantage of Redis' CRLF line terminators. No more readavailable required, since we know when the Redis output is finished.

(In particular, this includes fixes for #1, #6, #7, and #8.)

[sbromberger]

I did this as a proof of concept to show that we can cast Redis output to Julia types if we need to. time seemed like an obvious first choice.

[jkaye2012]

Would you mind moving this part into another pr for discussion? There's probably value in doing this, but I'd like to keep things consistent in the mean time.

[sbromberger]

No worries. give me a few mins.

[sbromberger]

Phew! I think that might do it, no?

[jkaye2012]

Looks pretty good to me. Let me pull it down quickly to run the tests, then I'll merge away.

Thanks for taking the time to help out on this!

[sbromberger]

Absolutely my pleasure. I have grand plans for Redis + LightGraphs.

[sbromberger]

Also, if you wouldn't mind tagging a new version, that way I can add this as a dependency in a project I've got going on.

PS: I took the liberty of creating a PR on Redis.io to change their "clients" page to point here for the Julia client. They don't currently have anything for Julia there.

[sbromberger]

PPS: I didn't make any changes to (or do any testing with) PubSub, so I don't know whether this impacts it.

[jkaye2012]

Sure, I'll tag the version once I merge it.

I think this change might break v0.3 compatibility though. When I run the tests, get fails with this message:

ERROR: type cannot be constructed
in map at abstractarray.jl:1328
in parse_bulk_string at /Users/jkaye/.julia/v0.3/Redis/src/parser.jl:26
in parseline at /Users/jkaye/.julia/v0.3/Redis/src/parser.jl:58
in execute_command at /Users/jkaye/.julia/v0.3/Redis/src/parser.jl:85
in get at /Users/jkaye/.julia/v0.3/Redis/src/client.jl:70

Perhaps we can just require v0.4+ or something. Thoughts?

[sbromberger]

Ah. Yes. This is a @compat thing, I think. Could you test the latest commit to see whether that fixes things? (I don't have 0.3 to test with at the moment.)

[jkaye2012]

Getting closer. A new one with Vector{Any}

ERROR: type cannot be constructed
in parse_array at /Users/jkaye/.julia/v0.3/Redis/src/parser.jl:35
in parseline at /Users/jkaye/.julia/v0.3/Redis/src/parser.jl:58
in execute_command at /Users/jkaye/.julia/v0.3/Redis/src/parser.jl:85
in keys at /Users/jkaye/.julia/v0.3/Redis/src/client.jl:70

[sbromberger]

Yup, this is JuliaLang/Compat.jl#105. Let me fix it. You'll want to change this once the Compat fix is in, though.

[sbromberger]

Can you retest with 0.3 now?

[jkaye2012]

Okay, pretty sure this will be the last problem we find.

Subscription is broken because of this line https://github.com/jkaye2012/Redis.jl/blob/master/src/client.jl#L49

Just have to plug the new parsing in there and everything should work. This would also be broken on v0.4; after it's fixed just run a local Redis server (with data that you don't care about - it will run flushall) and run julia test/redis_tests.jl to see if everything is good to go.

[jkaye2012]

Also, this highlights another problem, which is that the Travis-CI build isn't properly running the tests right now because runtests.jl doesn't have anything in it, so I will have to fix that at some point.

I'm pretty sure this commit actually breaks parser_tests.jl as well now that I think of it.

[sbromberger]

Okay, pretty sure this will be the last problem we find.

Hah - you've just guaranteed that it won't be. :)

Thanks for the testing instructions. I'd been doing Pkg.test() without realizing that runtests was empty :)

[jkaye2012]

Yeah.. it's kinda goofy because redis_tests.jl is really the one you need to know that everything is working, but it's really an integration test and not a unit test.

[sbromberger]

OK, I'm having anxiety about pubsub here, since SubscriptionMessage relies on a Reply type that I'm not using.

[jkaye2012]

That reply was being used for the old parsing. If I'm not mistaken, subscription only uses the actual result, so you should be able to just pass the result of your new parsing into SubscriptionMessage and everything should be fine.

The way it works is that the subscription sends an array response to the client. Your parsing will turn that into an array of strings, which the SubscriptionMessage will parse for use by the frontend.

[sbromberger]

function SubscriptionMessage(reply)
        notification = reply.response

The new parser just returns an int, string, or array depending on the result of the command. There's no structure with a response attribute.

[jkaye2012]

Yeah, so you can remove the notification = reply.response and just use reply directly (which will be an array - the same that reply.response used to be)

[sbromberger]

Ah, I got it. pubsub is always an array, with the first element being the notification type. OK. stand by...

[sbromberger]

I kept this commit separate (unsquashed).

[sbromberger]

tests not working: specifically,

@test x == ["hello, world!"]

[jkaye2012]

Missing conn.socket as second argument here I believe: https://github.com/sbromberger/Redis.jl/commit/cec86893a3ee63234d759d7d459326d992727c4c#diff-29a732d52d0661ba454242753a687486R50

[sbromberger]

Gah, of course. (it's my code; I should've caught that. Sorry!)

tests run now, though there are deprecation warnings, and I had a quick bugfix for an error in the script.

ETA: This bug should really be fixed by importing Base.keys.

[jkaye2012]

Let's fix those in another commit. For now, this looks good on v0.3 and v0.4 so I'll go ahead and merge it once the build passes.

[sbromberger]

Just to be clear: I fixed the test script (by changing keys to Redis.keys) in this PR. I'll follow up on merge with a "real" fix.

[sbromberger]

Thanks. Stand by for the fix for testing script.

[sbromberger]

See #11. BTW, thanks so much for your prompt review/feedback on this flurry of changes. :)