-
Notifications
You must be signed in to change notification settings - Fork 445
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
Extract loaders from ipv8_module_catalog.py #6182
Conversation
be593b8
to
50f1205
Compare
100a7ec
to
ada438d
Compare
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 believe that moving module (/community) definitions to their own files is a good idea. However, I observe that this PR creates more problems than it solves.
I'd strongly suggest closing this PR and drafting a more formal design in an issue first.
@qstokkink , I hoped to first discuss it with @drew2a , but you guys discussing the problem of modularisation here... |
@ichorid I agree that this warrants more in-depth discussion and a formal design. This is why I called to move this PR to an issue first, instead of turning this PR into a long discussion. I have not checked out the Dependency Injector framework. However, from my experience with Python plug-ins, we need to make doubly-sure that whatever we use (e.g., Dependency Injector) is compatible with a frozen environment (as created by PyInstaller). Ideally, we should have a list of all plug-in frameworks and weigh the pro's and con's. We'll be stuck with this for years probably, so this is not something we should decide on a whim. |
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.
With the shift in issue #1, dropping plugins, none of the points I raised matter anymore.
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.
Oops, I was too fast. This one still holds: https://github.com/Tribler/tribler/pull/6182/files#r659146652
I've set the comments that were related to plug-ins to "resolved".
0930248
to
5e99b28
Compare
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.
Nice 👍 Thanks for sticking with us through an entire lab vision revision.
Some future work notes:
- This breaks Gumby, specifically https://github.com/Tribler/gumby/blob/devel/gumby/modules/gumby_session.py
- The launcher/loader classes are made for plug-ins. Now that we're not doing plug-ins anymore, they can be removed.
I may need to do another PR with Dependency Injection as a result of today's meeting, which will remove the use of After that, the Gumby can be fixed. |
Continuation of the DependencyInjector discussion: #4953 |
5e99b28
to
6d99aaa
Compare
Kudos, SonarCloud Quality Gate passed! |
One step closer to the atomic community: introduction of
launcher.py
I've extracted
IPv8CommunityLauncher
for each Tribler's community fromipv8_module_catalog.py
and put them intolauncher.py
next to corresponding communities.Preconditions
Preconditions
have been separated fromLauncher
definition toLoader
implementation:community_loader.py
This allowed the use of top imports inside each loader (which should lead to easier navigation and more convenient work inside IDE).
launcher.py
Community loader
Community loader can be passed to
Session
constructor now:session.py
Therefore you don't need to cheat session to specify your own set of community launchers:
run_bandwith_crawler.py