Plugins: Fix to @hook / @run_hook mechanism #1669
Conversation
cculianu
added
Do Not Merge (Yet!)
The @hook decorator was registering functions globally for all plugins, even ones not decorated with @hook would get picked up. This change fixes it so that only functions decorated with @hook get picked up. This commit is not 100% ready for merge to mainline. I still have to do some due diligence and verify no extant plugins DEPENDED on the old behavior for correct operation!
What we were doing before could trigger an @Property instance property to execute a function as part of our getattr() check in _is_hook. This commit handles the property situation correctly.
This may be faster, and was suggested by Mark.
We just pick the first one and print a warning to the console.
|
Ok, I ran through all the plugins shipped with EC (plus a few extra popular plugins I know people use). I used fancy Conclusion: There are no hook function that lack So this change is safe to merge. |
cculianu
removed
the
Do Not Merge (Yet!)
very nice, thanks for doing this search! |
My OCD is satisfied.
| for name, func in self._hooks_i_registered: | ||
| l = hooks.get(name, []) | ||
| try: l.remove((self, func)) | ||
| except ValueError: pass # this should never happen but it pays to be paranoid. |
There was a problem hiding this comment.
these 2 lines still counts as 4 lines of code in my mind :-P
There was a problem hiding this comment.
Are you saying I should reformat it? or? I'm a C++ guy. It's hard for me to waste so many lines. I can do so.. if you suggest it more clearly. I'm not sure I understand.. ha ha.
There was a problem hiding this comment.
Hehe just bugging you, it's fine :D
|
Yeah this looks good 👍 |
|
note: 1 more commit added to alphabeticize the imports.. ahh. |
* Fix to plugin run_hook mechanism The @hook decorator was registering functions globally for all plugins, even ones not decorated with @hook would get picked up. This change fixes it so that only functions decorated with @hook get picked up. This commit is not 100% ready for merge to mainline. I still have to do some due diligence and verify no extant plugins DEPENDED on the old behavior for correct operation! * Follow-up to @hook fix: Deal with @Property attributes properly What we were doing before could trigger an @Property instance property to execute a function as part of our getattr() check in _is_hook. This commit handles the property situation correctly. * @hook fix follow-up: Check for _is_ec_plugin_hook on class-level This may be faster, and was suggested by Mark. * Nit: Allow run_hook hooks to return more than 1 result We just pick the first one and print a warning to the console. * plugins.py nit: Alphabeticized the imports My OCD is satisfied.
As per a Telegram chat with @markblundeberg -- we realized that:
The @hook decorator was registering functions globally for all plugins,
even ones not decorated with @hook would get picked up.
This change fixes it so that only functions decorated with @hook get
picked up.
This commit is not 100% ready for merge to mainline. I still have to dosome due diligence and verify no extant plugins DEPENDED on the old
behavior for correct operation!
EDIT: It turns out this PR is safe to merge. I ran through all the code and verified all hook-like-functions are indeed decorated with
@hook.