atlas-engineer / nhooks

MIT License
18 stars 5 forks source link

add-hook does not allow overriding existing handler #9

Closed Ambrevar closed 2 years ago

Ambrevar commented 2 years ago

Currently calling add-hook over a handler with an existing name is a no-op.

But what if we want to replace the handler in question?

Right now, the only way is to remove the handler then re-add it.

I wonder if it's really what's most expected. It seems to me that we would normally expect the latest version of a handler to be registered.

So I suggest reverting the behaviour: add-hook replace handlers with the same name, and if the user does not want it, they can just check for existence with find-handler.

Thoughts? @BlueFlo0d

kchanqvq commented 2 years ago

Makes sense. I'm a bit busy the following days but will be able to do it this week.

kchanqvq commented 2 years ago

Some design detail:

Addendum: This got me thinking about adding some keyword argument such as :before and :after to add-hook, so the user can control the order of handlers (e.g. they may want one handler to appear always after another). But I believe no such use case has shown up yet, maybe I'm over-designing.

Ambrevar commented 2 years ago

All good questions!

This also makes the code very simple!

That said, maybe we want to add an add-hook option to preserve position and state.

kchanqvq commented 2 years ago

That said, maybe we want to add an add-hook option to preserve position and state.

I think this option is not really necessary. If the user want to do it in place, they can just redefine the symbol function (the most common use case), or replace the fn slot of handler.

That said we should probably update find-handler? It looks a bit out of sync with the rest. Once it is taken care of, users can then (unless (find-handler ...) (add-hook ...)) to ensure a handler is present.

Ambrevar commented 2 years ago

Thanks!