-
Notifications
You must be signed in to change notification settings - Fork 35
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
Implementing a new importing scheme #789
base: develop
Are you sure you want to change the base?
Conversation
@cg-laser any thoughts? Do you like this idea or not so much? It does hide a little bit what things are - for me that is okay and the benefits in code writing out way this and over disadvantages (maybe there are more I am not aware of yet). @sjoerd-bouma do you have any thoughts as well? |
I really looks to me that we have bigger fish to fry than working on this ... |
I see the appeal of these changes. It cleans up the code (a bit) and avoids lengthy lines. The main disadvantage is that it requires "magic" init functions that provide somewhat magic functionality. It gets less obvious where a certain module is implemented. |
As I found with the module checker from #813, it is non-trivial to import all modules. At least one is broken on import (due to internal API change), and several require more complex dependencies (e.g. AraRoot or snowshovel). As far as I can tell, snowshovel is not even publicly available so there is no good solution there. Of course, you can add try around the imports and just ignore modules that don't work or have unmet dependencies, but you have to do that for each one, which is annoying. Another alternative is to use the import replacement from fuckit.py https://github.com/ajalt/fuckitpy?tab=readme-ov-file#as-a-replacement-for-import, though introducing a dependency on fuckit.py may encourage usage elsewhere... |
This is a guilty pleasure thing. I dislike the current scheme of importing modules and framework classes and they require to much code.
(For developers)
This PR adds imports into the
__init__.py
files of the packagesNuRadioReco.modules
andNuRadioReco.framework
. Here are two snippets:and
Notice that in the latter the name of the class objects is changed to have a capital letter as first letter. This does two things: It allows backwards compatibility (old import statements are still working) and it implements a typical syntax where classes use capital letters (similar as we do in the framework). However, it might introduce some confusion that the same classes are now accessible with two names (one with and one without the capital letter).
(For users)
This new import scheme allows the following code (pseudocode):
One major disadvantage is, that now all modules are always loaded which: I) might take more time (although maybe only the first time, afterwards with caching it should be relatively fast) II) makes it a bit more cumbersome to implement modules which use optional dependencies.
Currently I only modified
simulation.py
to use this new scheme. I think we could implement the new scheme step by step and do not have to do it all at once (since it is backwards compatible)