Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd support for auth to the constructor and add list push/pop with Rcpp based serialization of R objects #11
Conversation
| @@ -0,0 +1,21 @@ | |||
| #' redisConnect | |||
| #' | |||
| #' @import proto dplyr | |||
eddelbuettel
Oct 1, 2015
Owner
I don't think we need proto or dplyr here.
Also, should the optional password not be part of the constructor? See these lines accessed from here and made available to R here.
I don't think we need proto or dplyr here.
Also, should the optional password not be part of the constructor? See these lines accessed from here and made available to R here.
russellpierce
Oct 1, 2015
Author
Contributor
You're right, proto and dplyr are left overs from other code that I ripped out for the push.
You're also right that the optional password could be part of the constructor. Unfortunately, I speak R better than Cpp, so it was faster/cleaner/more likely to result in success for me to write this part of the code in R. I didn't think that dropping out to R for this part of the connection would have serious performance implications since connecting to the redis server would typically be a one-time affair.
Even if it is decided that it is mandatory to put the AUTH inside of the constructor, I would still encourage having an R function wrapper for connecting because it takes positional arguments and the way the function works seems more familiar to me from an class-impaired R-speaker standpoint than using the new(). But, mileage may vary.
You're right, proto and dplyr are left overs from other code that I ripped out for the push.
You're also right that the optional password could be part of the constructor. Unfortunately, I speak R better than Cpp, so it was faster/cleaner/more likely to result in success for me to write this part of the code in R. I didn't think that dropping out to R for this part of the connection would have serious performance implications since connecting to the redis server would typically be a one-time affair.
Even if it is decided that it is mandatory to put the AUTH inside of the constructor, I would still encourage having an R function wrapper for connecting because it takes positional arguments and the way the function works seems more familiar to me from an class-impaired R-speaker standpoint than using the new(). But, mileage may vary.
eddelbuettel
Oct 1, 2015
Owner
Nope. I breaks the entire design. It belongs into the constructor--every db accessor library or package on the planet does it that way as best as I can tell. So please remove the R function.
If you're unsure how to add to it to the C++ (re-read the three pointers I gave above) then I can do it. I would appreciate additional testing. Our Redis instances are password-free ...
Nope. I breaks the entire design. It belongs into the constructor--every db accessor library or package on the planet does it that way as best as I can tell. So please remove the R function.
If you're unsure how to add to it to the C++ (re-read the three pointers I gave above) then I can do it. I would appreciate additional testing. Our Redis instances are password-free ...
| @@ -611,7 +670,11 @@ RCPP_MODULE(Redis) { | |||
| .method("setVector", &Redis::setVector, "runs 'SET key object' for a numeric vector") | |||
| .method("getVector", &Redis::getVector, "runs 'GET key object' for a numeric vector") | |||
| .method("listLPop", &Redis::listLPop, "pops numeric vector to list") | |||
| .method("LPop", &Redis::LPop, "pops R object to list") | |||
| .method("RPop", &Redis::RPop, "pops R object to list") | |||
eddelbuettel
Oct 1, 2015
Owner
Maybe add 'left' and 'right' to the descriptions?
Also, maybe these two (and the ones for push) belong a few lines higher into the 'R serialization' block?
Maybe add 'left' and 'right' to the descriptions?
Also, maybe these two (and the ones for push) belong a few lines higher into the 'R serialization' block?
russellpierce
Oct 1, 2015
Author
Contributor
I believe I have moved them where you indicated. Now that I see them in context, I noticed that my description field is inconsistent with the others in this section. I believe my version reads cleanly enough, but would you prefer that I standardize the language to fit your other methods (or vice-versa)?
I believe I have moved them where you indicated. Now that I see them in context, I noticed that my description field is inconsistent with the others in this section. I believe my version reads cleanly enough, but would you prefer that I standardize the language to fit your other methods (or vice-versa)?
drop unnecessary references to proto and dplyr and use the constructor (thanks Dirk)
|
Really appreciate it. Will take a closer peek in a bit. |
|
PR looks lovely now for the part covering I would still prefer not to add the change to |
russellpierce
changed the title
|
Sorry, I'd misunderstood. I thought your objection was about where/how I had instantiated the auth, not the wrapper itself. I think I've successfully reverted the changes associated with the redisConnect function and so you should just have the Redis.cpp changes left in the PR. |
|
Thanks so much -- I was actually looking at this earlier. Folding it in now, and will add a ChangeLog entry for too in a minute. One more thing that would be nice ... see |
Add support for auth to the constructor and add list push/pop with Rcpp based serialization of R objects
eddelbuettel
merged commit fe5017a
into
eddelbuettel:master
also rolled (minor dev) Version and Date
|
And I did one small set of edits to standardize names a little:
See e3103c6 |
I don't know if you are considering accepting pull requests on this project given its stable status. However, in case you are, I thought I'd give it a shot. The comments made it seem as though you weren't sure which way to jump in terms of serialization, so I just took a stab in the direction I thought most would find more useful (understandably limited to my particular use case).