Skip to content

Conversation

travishathaway
Copy link
Contributor

@travishathaway travishathaway commented Aug 2, 2022

This pull request was largely inspired by this pull request: #11435

What I am trying to do differently here is to give plugin authors a way to define a command line interface for their sub-commands. This is accomplished by telling argparse in conda not to parse arguments for sub-commands it knows to belong to plugins.

This is accomplished by overriding the parse_args method on our custom ArgumentParser class:

https://github.com/travishathaway/conda/blob/9277f1e439f890bbbf557499d4045f646bb76034/conda/cli/conda_argparse.py#L228-L236

If it's a known plugin, we simply do nothing. Under the hood, it simply checks the value of sys.argv[1] to see if that string matches a name in our registered plugins list.

Later in do_call:

https://github.com/travishathaway/conda/blob/9277f1e439f890bbbf557499d4045f646bb76034/conda/cli/conda_argparse.py#L107-L113

We perform a naive check (maybe there's better way to do this?) to see that the args object doesn't have a func attribute. If doesn't we proceed to find the sub-command and execute its action callable.

Because this just calls a function without having parsed any arguments thus far, this leaves us free to do whatever kind of argument parsing we want. Below is an example program using click as the argument parser:

import click
import conda.plugins

__version__ = "0.1.0"

SUMMARY = "Example implementation of conda-doctor command"
SUBCOMMAND_NAME = "doctor"


@click.group()
def cli():
    pass


@cli.command(SUBCOMMAND_NAME)
@click.version_option(__version__, prog_name="conda-doctor")
@click.argument('param_one')
def command(param_one):
    print(param_one)


@conda.plugins.register
def conda_subcommands():
    yield conda.plugins.CondaSubcommand(
        name=SUBCOMMAND_NAME,
        summary=SUMMARY,
        action=cli
    )


if __name__ == '__main__':
    cli()

The click program needs to be setup as-if it was a program using sub-commands which is why we define a click.group. Otherwise, it works just like you would expect it.

beeankha and others added 29 commits July 27, 2022 21:03
…this change, update tests that call the generate_parser() function
Co-authored-by: Filipe Lains <lains@riseup.net>
Co-authored-by: Filipe Lains <lains@riseup.net>
Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com>
Co-authored-by: Filipe Lains <lains@riseup.net>
Co-authored-by: Filipe Lains <lains@riseup.net>
Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>
Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>
Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>
Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>
Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com>
Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com>
Co-authored-by: Katherine Kinnaman <kkinnaman@anaconda.com>
…comprehension, find all possible subcommand conflicts
@travishathaway travishathaway added the type::feature request for a new feature or capability label Aug 2, 2022
@travishathaway travishathaway added the source::anaconda created by members of Anaconda, Inc. label Aug 2, 2022
@travishathaway travishathaway requested a review from a team as a code owner August 2, 2022 23:57
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Aug 2, 2022
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the idea for the initial plugin implementation was to stay close to the previous conda-* API and have a similar behavior and specifically not provide an argparse specific plugin hook.

That was a design decision to reduce the effort needed for existing packages extending conda with conda-* scripts to have a transition path and gain some experience before we support more advanced techniques like supporting click etc.

I've left a few comments and think this still has value FWIW, e.g. we could move this into an own ArgparseCondaSubcommand class that could be an β€œadvanced” option for plugins with more complex needs. That said, generally speaking, I would caution us to leaning into argparse given its obvious API problems. I'd rather have us work towards supporting tools like click or typer.

raise CondaError(
"There is something mis-configured with your subcommands. This could be due to an "
"incorrectly configured plugin. Please try uninstalling any problematic plugins "
"you may have to remove this error."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help to print the args here so users can identify what is not working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, definitely would be nice to have some detailed debugging information here just like the error report we normally show.

This time we have an implementation that fully supports any CLI parsing
library you may want to use. It does by modifying the ArgumentParser
class to add a "pass-thru" for plugin subcommands. This simply means
that we add a stub for the command and then pass it off to the plugin
for actual command line argument parsing.
@kenodegard kenodegard added the in-progress issue is actively being worked on label Aug 10, 2022
@kenodegard kenodegard self-assigned this Aug 10, 2022
@kenodegard kenodegard removed their assignment Aug 23, 2022
@beeankha beeankha removed the in-progress issue is actively being worked on label Aug 24, 2022
@jezdez
Copy link
Member

jezdez commented Jan 24, 2023

@travishathaway is it okay to close this?

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jan 27, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity plugins pertains to a plugin/subcommand source::anaconda created by members of Anaconda, Inc. type::feature request for a new feature or capability
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants