- Joined
- Jul 10, 2009
- Messages
- 534
The new approach looks interesting. I think I like it, although the
A few things to consider:
Hook:<name>
syntax has unnecessary restrictions in comparison to AddHook
(doesn't support priorities and host tables).A few things to consider:
- You are using object-oriented style now, but it's unclear what your objects are. Doesn't feel good for the user to write
self
without understanding what that stands for. - Traditionally, colon-notation is used for methods (on class objects), and dot-notation for library functions and class functions. I find it confusing that
old
andremove
use different notation, although they belong to the same type. - Maybe rename
AddHook
toHook.add
now where you have a class for it? You can keep AddHook for legacy reasons ofc. - You could add a proper class-annotation with all methods available (add, old, remove) to support the user.
- It's unclear to me how to use
old
andremove
inside a normalAddHook
. Hook(...)
looks confusing (and even moreso after reading your description), what are reasons to use it? If you don't find an answer, probably better deprecate it.self.old
sounds like I would call the original native, while in reality I call the next hook. Maybeself.next
would be a more fitting name. Or maybe provide both?- You seem to use
rawset
to write into the host table. Why is that? Host tables relying on__index
and__newindex
could be corrupted this way. AddHook
returnsself.old
, which seems to be the predecessor of the new hook. The real predecessor however can change at any time (new hook added in between, predecessor removes itself, ...). What's the sense behind returning it anyway? Is the user supposed to do anything with it?- You are using
error
to print an error message, which will only work in a protected call (but still crash the thread in a non-protected call without the user being notified). Maybe useprint
instead, if you just want to inform the user?
Last edited: