MichaelDrogalis / dire

Erlang-style supervisor error handling for Clojure
482 stars 20 forks source link

Complementary remove functions #22

Closed dparis closed 11 years ago

dparis commented 11 years ago

Created a set of complementary functions which remove each type of dire hook. As well, I added a function remove the supervisor hook altogether from the target function. By keeping it a separate function, users now have the flexibility to add and remove all dire hooks at runtime, while retaining control over whether the function still uses the baked-in supervisor. This also means that a function which had a with-* bang method applied can disable the baked in hook while retaining the registered hook.

By the way, I changed the key being used to register the robert.hooke hook. It was currently using a non-namespaced :key. I changed it to a namespaced ::supervisor-hook-key which should play nice with any app that uses robert.hooke for anything aside from dire.

It's all pretty thoroughly tested, but if you think of any test cases I missed, I'd be happy to add them. Give it a spin!

dparis commented 11 years ago

Oh, and if you want me to bump the version number or something in the PR, just let me know to what.

dparis commented 11 years ago

Double-oh, just FYI, I pulled in the core.incubator for their implementation of dissoc-in. If you want to keep it out, I can just copy-paste the method def into dire.core, it's pretty small.

MichaelDrogalis commented 11 years ago

Great stuff, thanks for doing this! I'll take a closer look tonight and merge in. :)

MichaelDrogalis commented 11 years ago

Thanks for this! I don't want to deploy just yet. This is a case that I can easily see someone getting confused:

(defn f [x] (inc x))

(defn a [y]
  (prn "A:" y))

(defn b [y]
  (prn "B:" y))

(with-pre-hook! #'f a)
(with-pre-hook! #'f b)

(f 1)

(prn "***")

(remove-pre-hook #'f a)

(f 1)

Obviously hook a isn't cleared, and both hooks remain. Since there aren't any banged versions of remove, users would probably think this set of remove functions serve both purposes. I think we should try to create a set of banged remove functions too. Removing the supervisor hook entirely is too much in some cases. For example, if I had a hook and a handler, and just remove to remove the handler and keep the hook - I can't do that.

dparis commented 11 years ago

Yeah, I wondered about how to handle this. In fact, I may have misunderstood how it works, currently. I was under the impression that the embedded supervise hook was essentially separate from the various handlers. By adding/clearing the supervise hook, you only change the need to call supervise. All of the handlers use a separate mechanism.

As such, wouldn't the sample code above leave hook b in place, along with the baked-in supervisor call? I'm not at my workstation, so I can't test it to verify the current behavior, but I would guess the final call to f would call the b pre-hook and then execute f. If one were then to add a line calling (remove-supervisor #'f) at the end, the b pre-hook would still be registered, but the caller would need to wrap further f calls in a (supervise ...) for it to fire.

In any case, coming up with matching bang methods is fine by me, but I think there ought to be a consistent behavior regarding the supervisor hook. It makes the most sense to me that a remove-*! function would remove the registered handler and then, if no dire handler types are found, clear the supervisor hook.

MichaelDrogalis commented 11 years ago

Agreed about your last sentence.

MichaelDrogalis commented 11 years ago

By the way, the result of those two function invocations are:

"B:" 1
"A:" 1

And

"B:" 1
"A:" 1
MichaelDrogalis commented 11 years ago

I think we can get around this for banged versions by removing the supervisor hook, removing the relevant handler, and adding the supervisor hook back in.