-
Notifications
You must be signed in to change notification settings - Fork 29
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
Plugins revamp #134
Plugins revamp #134
Conversation
@rlizzo There will be few more minute changes but it's in a good shape now to review especially because of the introduction of |
Ok. I'd like to merge the changes in #130 before this, since they did a big overhaul of the test suit to simplify it's fixture setup (improving speeds in most cases) |
Make sense. I'll finish my review soon |
Thanks! |
Hey @hhsecond lets connect about these changes on a call. I don't think I agree with moving to |
Sure thing. I was reluctant too about it initially but then realized few reasons why should we do it. Let's talk |
Codecov Report
@@ Coverage Diff @@
## master #134 +/- ##
==========================================
+ Coverage 95% 95.15% +0.15%
==========================================
Files 63 61 -2
Lines 11203 11189 -14
Branches 998 968 -30
==========================================
+ Hits 10643 10646 +3
+ Misses 372 363 -9
+ Partials 188 180 -8
|
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.
So i'm a bit baffled by the control flow here. There's some nice ideas (the cli changes seem to be really solid), but the manage_plugins.py
file is a mess. Can you walk me through how exactaly plugins are discovered, loaded, and executed?
What's the actual use case for plugins? I'm worried it's a lot of complexity and API surface area for benefit that isn't super clear from the outside. |
@elistevens comment here #134 (comment) should give you more info. Lemme know what do you think |
Sounds great!
…On Fri, Oct 18, 2019, 23:07 Sherin Thomas ***@***.***> wrote:
@elistevens <https://github.com/elistevens> comment here #134 (comment)
<#134 (comment)>
should give you more info. Lemme know what do you think
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#134?email_source=notifications&email_token=AABBWICFKAAAKUGWM3EQM6LQPKPZVA5CNFSM4I7YWL2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBXGL6Y#issuecomment-544105979>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBWICCQQI3VPQJX5TA3PDQPKPZVANCNFSM4I7YWL2A>
.
|
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.
Overall this is a really good design. There's a few things to clean up, but I really like this! Nice Job!
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.
These and my previous single comments need to be changed. There are some serious errors and code smells in the cli methods, the tests need to be much more thorough, since they seem to have missed some really big issues.
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 looks good to me. when the ci suite run's i'll merge!
…her changes requested upon review
…is not extra require and more
Motivation and Context
Revamping plugin system to make at actually pluggable for different data types etc.
Description
io
module. This will help users to use the import/export functionality of plugins through the program if they don't wan't to interact with the low-level hangar APIsdataset
insideio
moduleTypes of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Is this PR ready for review, or a work in progress?
How Has This Been Tested?
Put an
x
in the boxes that apply:Checklist: