add pool for pubsub connections

[bbaldino]

Based on discussion in #70

[bbaldino]

Something needs to be done here, but not sure what would be best for a PubSub.

[djc]

Maybe punsubscribe() with an empty pattern?

[bbaldino]

Good idea. Made that change, just trying to validate a few things.

[bbaldino]

Ok, pushed the logic for is_valid. Unfortunately I wasn't able to really validate some things in my use case (due to other issues), and didn't see any unit tests in the redis that I could expand on. If there's anything you think might be risky here (bug-wise) we can hold off.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -1.83 ⚠️

Comparison is base (040b7dc) 74.28% compared to head (1846e2d) 72.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   74.28%   72.45%   -1.83%     
==========================================
  Files           6        6              
  Lines         556      570      +14     
==========================================
  Hits          413      413              
- Misses        143      157      +14     

Impacted Files	Coverage Δ
redis/src/lib.rs	3.12% <0.00%> (-2.44%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[djc]

Ok, pushed the logic for is_valid. Unfortunately I wasn't able to really validate some things in my use case (due to other issues), and didn't see any unit tests in the redis that I could expand on. If there's anything you think might be risky here (bug-wise) we can hold off.

I mean, if you can't validate this stuff, why are we considering this for inclusion?

[bbaldino]

Ok, pushed the logic for is_valid. Unfortunately I wasn't able to really validate some things in my use case (due to other issues), and didn't see any unit tests in the redis that I could expand on. If there's anything you think might be risky here (bug-wise) we can hold off.

I mean, if you can't validate this stuff, why are we considering this for inclusion?

Well, the basic functionality (retrieving a connection) does work, it's the is_valid I haven't been able to specifically test. I did say that if we want to hold off on merging until I get a chance to more directly test that part, then by all means let's do that, but:

1. There are no unit tests in bb8_redis, so for all I knew maybe this isn't something you were all that worried about.
2. The code itself is pretty simple, perhaps by reading it alone you (who has more experience than me) are able to feel confident about its correctness.

So was merely trying to defer to you on how you wanted to move forward.

[djc]

Even though we have no tests, given that the entire idea of using punsubscribe this way is sort of experimental, I would prefer to see if it works in practice at least once before merging this.

[bbaldino]

Sorry for the lack of follow-up here...we ended up going in a different direction (no longer using redis due to other reasons), so I likely won't have time to come back to this.

[djc]

No problem, thanks for following up! I'll close this -- if someone else wants it, please submit a new PR.