Skip to content
This repository has been archived by the owner on Mar 19, 2018. It is now read-only.

making redis wrapper handle nil timer. adding missing regigo api passthroughs #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

captncraig
Copy link

currently if I call conn.DoTimer with a nil timer (perhaps call is not from a wrapped http request), it will panic. This adds parity with things like profile.StepCutomTiming, which still passthrough if an inappropriate timer is used.

Also added some newer additions to redigo.

@maddyblue
Copy link
Member

The redis package works and is still a good idea? I was planning on removing it because I assumed it was old and busted. But ok.

@captncraig
Copy link
Author

captncraig commented Oct 23, 2016

Umm, to be honest, I just started using it. I kinda needed this pr to even make my first experiments work.

If I was doing this from scratch, the "pretend to be the redis package, but do a few things differently" approach would definitely not be my first choice.

Perhaps something simple like:

type WrappedConn struct{
   redis.Conn
   t miniprofiler.Timer
}
func (wc *WrappedConn) Do(...){
   //time and passthrough to wc.Conn.Do
}

would be sufficient. Along with a similarly simple pool wrapper to generate those from a timer:

type WrappedPool struct{
   p *redis.Pool
}
func (w *WrappedPool) Get(t miniprofiler.Timer) redis.Conn

or something along those lines.

Would that kind of api make more sense. If you were gonna delete this anyway, should I make a bigger overhaul to that effect? Are there any known users of this package in the wild?

@maddyblue
Copy link
Member

There are no known users (I never even used it). I originally added all the other addon packages to try to make adoption easier, but I was also a new go programmer then. I'd just as soon nuke all the other packages and start over with something more reasonable. Or perhaps just add documentation about correct ways to use redis, if there's no need for a full package or something.

@maddyblue
Copy link
Member

I've invited you for write access to this repo so you can do whatever you want with this PR and stuff.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants