-
-
Notifications
You must be signed in to change notification settings - Fork 179
Introduce Subsystem Component Registration #769
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
base: main
Are you sure you want to change the base?
Conversation
ffc7b83 to
9b43ceb
Compare
This change adds a new component registration mechanism that allows modules to register named components with one or more subsystems. The registration happens before printer object creation so no access to Printer is required, limiting possible side effects of registration. This creates a way to do truly dynamic registration without having registries in code, special directories, or files with lists of components. Modules can get registered components at `load_config`\`load_config_prefix` time with a call to `Printer.lookup_components`. Most of the PR is re-working `Printer.load_object` so that it can use a cache of loaded modules. The cache is built prior to any printer objects being created. Having the cache allows for all modules to be scanned for a `register_components` function which is called before the config file is processed. Signed-off-by: Gareth Farrington <[email protected]>
9b43ceb to
d670a74
Compare
| m.add_early_printer_objects(self) | ||
|
|
||
| @staticmethod | ||
| def _list_modules(search_path: str) -> list[Path]: |
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.
This isn't the right PR to change this, since you're mostly refactoring existing code here, but I don't think this is the best way of going about finding modules -- it'll miss .pyc, .so, and doesn't work for stuff like a ZipImporter. pkgutil.iter_modules looks like it'd be much more robust.
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 will look at pkgutil.iter_modules. This is the PR to make that change, lets get it right the first time.
| if subsystem_name not in self._subsystems: | ||
| return {} | ||
| return self._subsystems[subsystem_name] |
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.
| if subsystem_name not in self._subsystems: | |
| return {} | |
| return self._subsystems[subsystem_name] | |
| return self._subsystems.get(subsystem_name, {}) |
But see also my comment about DefaultDict, in which case you can just
| if subsystem_name not in self._subsystems: | |
| return {} | |
| return self._subsystems[subsystem_name] | |
| return self._subsystems[subsystem_name] |
| if subsystem_name not in self._subsystems: | ||
| self._subsystems[subsystem_name] = {} | ||
| subsystem = self._subsystems[subsystem_name] |
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.
Personal preference maybe, but consider collections.DefaultDict for registries like this. It has its problems, though... But it makes these check-and-set patterns a lot more concise.
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.
This is very basic use case, so it should work fine.
| ENDSTOP_REST_TIME = 0.001 | ||
| ENDSTOP_SAMPLE_TIME = 0.000015 | ||
| ENDSTOP_SAMPLE_COUNT = 4 |
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 don't know this code well and haven't stepped through all the ins and outs, so this may be a dumb comment, but ... it really looks like these should now no longer be globals, but instead instance variables populated during load_config.
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.
Agreed. I was trying to keep the spirit of the existing code but making them local lookups makes more sense.
This change adds a new component registration mechanism that allows modules to register named components with one or more subsystems. The registration happens before printer object creation so no access to Printer is required, limiting possible side effects of registration. This creates a way to do truly dynamic registration without having registries in code, special directories, or files with lists of components.
Modules that want to register a component expose a
register_components()function e.g.:Modules can get registered components at
load_config\load_config_prefixtime with a call toPrinter.lookup_components. e.g.Most of the PR is re-working
Printer.load_objectso that it can use a cache of loaded modules. The cache is built prior to any printer objects being created. Having the cache allows for all modules to be scanned for aregister_componentsfunction which is called before the config file is processed.