-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Plugin system implementation #11121
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
Plugin system implementation #11121
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.
Thanks @FFY00, this is a great start, thank you! I've left a few notes, but nothing blocks this from my perspective.
I have been having some issues with the tests, it seems things get cached somewhere 😕 |
Seems like the argparse instance is cached 🤦 |
5c2b067
to
7c15834
Compare
The latest commit (416f348) implements virtual packages via the plugin system. Virtual packages defined by conda are implemented as core plugins, which exist under the |
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Instatiating the parser is cheap and should only happen one time in cases where performance matters anyway. Caching this makes the tests more complex and, I believe, just adds unnecessary maintenance cost. Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
* docs: add plugin API Signed-off-by: Filipe Laíns <lains@riseup.net> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update docs/source/plugin-api/index.rst Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com> * Update docs/source/plugin-api/index.rst * Update docs/source/plugin-api/index.rst * Update docs/source/plugin-api/index.rst * Update docs/source/plugin-api/index.rst Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jannis Leidel <jannis@leidel.info> Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com> Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Closing, now that #11435 has been merged into the plugins feature branch. |
This got a bit delayed with Christmas, and then I fell ill, but it is here at last.
The plugin system itself is fairly simple, it essentially consists of a few definitions in a new
conda.plugins
module.Initially, I planned to only expose the plugin manager instance to the rest of the conda codebase via the context object, but that proved insufficient right on the first plugin implementation, as the argument parsing happens before the context object is created, and the context object creation is currently inherently dependent on the argument parsing. As such, the plugin manager is also exposed via a helper function in the context module. This might limit us in the future, but that could potentially be solved by refactoring the context object a bit.
Instead of exposing the plugin manager directly, I think we might benefit of having a wrapper around it that does all the data necessary gathering, but this is not clear to me at this point, hopefully it will become clearer as we implement more plugins and get an idea of their needs.
Overall, I think the code size is pretty small, which was my goal. It demonstrates how easily plugins can be implemented in the conda codebase.
Regarding the actual subcommand plugin implementation, I did explore improving things a little bit more in regards to the
argparse
API usage and how to plug this functionality into it, but ultimately settled on just settingepilog
instead of overwritingprint_help
. Part of the reasoning was that jezdez expressed the wish to move away fromargparse
to a different module, likeclick
, so spending more time trying to improve this design did not seem worth it. For the same reason, and backwards compatibility, I decided to give plugin users full control over the argument parsing, instead of locking them into a specific API, like integrating the hook directly intoargparse
I still need to add the tests, but I think I am reasonably happy with the end result so far.