-
Notifications
You must be signed in to change notification settings - Fork 45
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
Mixin class #692
Mixin class #692
Conversation
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.
I think this general structure makes sense. I wonder if there could be a more informative name of the mixin (SpyglassMixin doesn't exactly tell you what it is doing), but I guess the class is broad enough (fetch nwb, delete) that Spyglass is okay.
I agree. Happy to rename if you have something else in mind, but I figured it would grow in responsibility over time - fetch_nwb and permission-restricted delete are already diverse enough responsibilities that I thought they would need separate mixins if we wanted informative names |
I think this is a good idea and will clean up a good amount of code. However, a minor suggestion - I think the class variable |
Description
Initial implementation of Mixin class is added to any table with an existing
fetch_nwb
, allowing special cases to override. Mixin uses table definition to decide which table to use. If none is in definition, class must have a table attribute. Is this enough to allow all tables to have the mixin, without tables inadvertently accessing the method?Checklist:
CITATION.cff
CHANGELOG.md