search

richfitz / redux

:telephone_receiver::computer: Redis client for R
https://richfitz.github.io/redux
91 stars 17 forks source link

i40: Support for Redis 6.2.0 #41

Open richfitz opened 4 years ago

richfitz commented 4 years ago

Adds support for new commands added through to Redis 6.2.0, incorporating changes in #28, fixing failing tests and adding tests for the new functions.

The interface to the stream (X*) functions I am not immediately happy with, as things like XREADGROUP I am not sure are properly supported yet, and commands like XGROUP really feel like they should be split into subcommands. It might be easiest to fold this in for now and then fix, but to do this properly I probably will have to either understand the streaming interface or get input from someone who has used it (@dseynaev perhaps?).

Fixes #40

codecov-io commented 4 years ago

Codecov Report

Merging #41 (8ba428e) into master (83b0614) will decrease coverage by 0.60%. The diff coverage is 95.41%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #41      +/-   ##
===========================================
- Coverage   100.00%   99.39%   -0.61%     
===========================================
  Files           18       18              
  Lines         1476     1642     +166     
===========================================
+ Hits          1476     1632     +156     
- Misses           0       10      +10
Impacted Files Coverage Δ
R/redis_api_generated.R 98.87% <95.39%> (-1.13%) :arrow_down:
R/util_assert.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 83b0614...8ba428e. Read the comment docs.

r2evans commented 3 years ago

The interface to the stream (X*) functions I am not immediately happy with, as things like XREADGROUP I am not sure are properly supported yet, and commands like XGROUP really feel like they should be split into subcommands.

IIUC, are you suggesting that there might be a set of subcommands not normally available in r <- hiredis() that would allow generation of a consumer group and only then provide relevant subfunctions?

r <- hiredis()
mygrp <- r$consumer_group("mystream", "mygroup", "myname")
mygrp$XADD(...)
mygrp$XREAD(...)

(Perhaps mangled a bit.) I don't like that interface, to be honest.

I think the most straight-forward path for redux would be to provide a "simple" flat API, supporting the X* functions exactly the same as the others. The fact that some of them accept arbitrary numbers of arguments (e.g., topics and offsets) is something that can be managed with a little bit of work. If there is any encapsulation of these basic functions into a more OO architecture, I argue it can be done on top of the vanilla functions.

My vote: add XADD and friends as-is. If there are issues with order of arguments (optional), then I don't see them yet, and don't know what errors I'm making in this recommendation.

kevinykuo commented 3 years ago

+1 that the streaming features should be exposed at the level consistent with existing functions, and sugar can be built on top of them later, either in this package (with a different naming scheme ideally) or elsewhere.

richfitz commented 3 years ago

Thanks @kevinykuo and @r2evans for the feedback. I will pick this back up as soon as I have time again for personal open source work (I've been knee-deep in covid work since mid-November with no spare time)

codecov-commenter commented 3 years ago

Codecov Report

Merging #41 (29ee6f6) into master (1756f97) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 29ee6f6 differs from pull request most recent head 8251240. Consider uploading reports for the commit 8251240 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##            master       #41    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           18        18            
  Lines         1479      1722   +243     
==========================================
+ Hits          1479      1722   +243
Impacted Files Coverage Δ
src/conversions.c 100.00% <ø> (ø)
R/redis_api_generated.R 100.00% <100.00%> (ø)
R/util_assert.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1756f97...8251240. Read the comment docs.

richfitz commented 3 years ago

Support up to ~6.0.0 is ok, but after that we get a new block argument type that I need to handle:

See https://github.com/redis/redis-doc/pull/1443 https://github.com/mmkal/handy-redis/pull/255

r2evans commented 1 year ago

@richfitz, I recognize that redux works well enough for your needs, and you haven't had time to prioritize towards it in the last couple of years. Have you considered allowing anyone else access to accept PRs? I admittedly don't have a lot of bandwidth, but perhaps I can provide just enough help to allow some much-needed (for others) updates?