-
Notifications
You must be signed in to change notification settings - Fork 693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add function to unregister a previously registered hook. #12100
Add function to unregister a previously registered hook. #12100
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but before official approval, a question:
Are there any tests for the existing code? Just thinking it would be good to maintain same level of coverage on this module if there are
and issubclass(parent, KolibriHook) | ||
and parent is not KolibriHook | ||
and hasattr(parent, "_registered_hooks") | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker: some of this logic seems like it's getting a bit repetitive. There are subtle differences, but bulk of it seems like it could be consolidated. Every time I look at this area of the codebase, it takes me a while to carefully understand all of the conditions. Some consolidation can tuck that a way into a well commented chunk of code, so each usage can focus on its goal and what's different
and base.abstract | ||
and issubclass(base, KolibriHook) | ||
for base in subclass.__bases__ | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here regarding consolidation. Seems this validation logic is pretty much the same as register_hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me, code is the same applied in https://github.com/learningequality/kolibri-data-portal/pull/682/ and it's already demostrating its utility in KDP in production.
As @bjester mentioned, tests would be a good to have, as usual, but I'm not going to be the one complaining for lack of testing. I'll love the day AI can create real and useful tests for us.
As I (re)-discovered when looking at this, there are no existing tests for this part of the code base - at least part of that is on me, as I didn't write any when I refactored this about 4 or 5 years ago. I am also happy to hold off merging to add some, I'll think on it. |
If there is no existing test coverage, then no worries, we can merge and add tests later. |
Summary
References
This was vendored into KDP in https://github.com/learningequality/kolibri-data-portal/pull/682
Reviewer guidance
Use it to unregister an otherwise previously registered hook.
Check that it is no longer a registered hook on its parent hook.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)