- Joined
- Jul 10, 2009
- Messages
- 541
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
selfwithout 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
oldandremoveuse different notation, although they belong to the same type. - Maybe rename
AddHooktoHook.addnow 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
oldandremoveinside 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.oldsounds like I would call the original native, while in reality I call the next hook. Maybeself.nextwould be a more fitting name. Or maybe provide both?- You seem to use
rawsetto write into the host table. Why is that? Host tables relying on__indexand__newindexcould be corrupted this way. AddHookreturnsself.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
errorto 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 useprintinstead, if you just want to inform the user?
Last edited:



