pip icon indicating copy to clipboard operation
pip copied to clipboard

Moving off optparse

Open pradyunsg opened this issue 7 years ago • 47 comments

@pypa/pip-committers Thoughts on switching over to using argparse or even click for option parsing?

pradyunsg avatar Aug 09 '17 14:08 pradyunsg

Since optparse is deprecated it would make sense to switch to argparse. But I think @dstufft was planning to switch to click.

xavfernandez avatar Sep 25 '17 21:09 xavfernandez

+1 for click from me.

I'm pretty sure there'd be some edge case the switch would break but that's fine.

pradyunsg avatar Oct 03 '17 19:10 pradyunsg

I think you would be better off with argparse as it's in the standard library and you don't have to add a new dependency to it. It also has the argcomplete library which is good for tab completion.

Also, read this and see what you think: http://xion.io/post/programming/python-dont-use-click.html

thechief389 avatar May 05 '18 20:05 thechief389

Summary of the blog post:

click has an hands-off approach and the author feels that approach isn't appropriate when the CLI is your user interface.


That's a fair argument, made in a fairly emotional tone IMO. (maybe that python-dev thread making me look at things this way)

I'm happy with both options and likely argparse is going to be easier to switch to anyways.

pradyunsg avatar May 06 '18 09:05 pradyunsg

it should be noted that argparse has a few pretty nasty "bydesign" bugs around automatically aliasing the prefix pf an argument to the full version, its only opt out in 3.6* and there is no fix for older pythons

RonnyPfannschmidt avatar May 06 '18 11:05 RonnyPfannschmidt

But nevertheless, argparse is the best choice.

thechief389 avatar May 06 '18 18:05 thechief389

@RonnyPfannschmidt do you have any links? My Googling couldn't find anything... :/

pradyunsg avatar May 06 '18 18:05 pradyunsg

https://docs.python.org/3/library/argparse.html#allow-abbrev - sorry for quoting the python version wrong, it was added in 3.5

RonnyPfannschmidt avatar May 07 '18 05:05 RonnyPfannschmidt

I'm working on moving to click. Is there anyone else doing it?

thechief389 avatar Nov 23 '18 17:11 thechief389

I've done a bit of work in the past but ran out of time; mostly was just trying to figure out what all is worth investigating, how things could be done and potential improvements before actually starting working on code.

I do have some notes I can share -- none of what's there in the notes is set in stone but might be a good place to start from. You don't need to respond to every hypothetical in it but those should be figured out as we do this. :)

Please feel free to discuss what you're thinking here.


Investigate:

  • Configuration interaction
    • Context Defaults?
      • Does the behavior map exactly?
  • How click's unicode support affects us: https://click.palletsprojects.com/en/7.x/python3/
  • Utilizing the *_option decorators: Is it worth the effort to bodge-use them when we're not doing decorators?
  • Argument? Or Context.allow_extra_args? Which is better for us?
  • How do we use Context in our context? ;)
  • Backwards compatibility?
    • Do we want an "opt-in", "opt-out", "drop" approach here? That's a 3 release cycle removal.
    • But look at how much might break via tests to determine the exact flow.
  • Using click's utilities (in place of our own/through dependencies)
    • get_terminal_size
    • prompt
    • progressbar?
    • edit? Maybe?
    • echo? I doubt.
    • style? Can do but should we?
    • get_app_dir? I doubt.
    • echo_via_pager --> should help with #5746
  • click-contrib has some fancy things
    • https://github.com/click-contrib/click-completion: Yes.
    • https://github.com/click-contrib/click-man: should do
      • How to replace existing man page generation? Ping stakeholders on original PR/issue to ask for feedback.
    • https://github.com/click-contrib/click-plugins: BIG FEATURE request -- look at this if we do go that route sometime.
    • https://github.com/click-contrib/click-configfile: Is this relevant to us?
    • https://github.com/click-contrib/click-didyoumean: Replace our custom logic?

Plan:

  • Rework the existing command setup to have the same "structure".
  • No decorators.
    • Call a method / list of Option in pip.commands? pip.cli.cmdoptions?
  • Do A/B testing: use an env var to have conditional for how they're parsed
    • need to edit tox to pass whatever env var

To be used parts of the API:

  • Command
  • Group
  • Option
  • Context? (see above)

Avoid direct use:

  • ASIDE: why aren't these marked more visibly / separated better? File an issue?
  • CommandCollection
  • Parameter
  • BaseCommand
  • MultiCommand

pradyunsg avatar Nov 23 '18 22:11 pradyunsg

Hey @thechief389! Have you made any progress on this?

pradyunsg avatar Dec 02 '18 10:12 pradyunsg

I had trouble trying to vendor all the libraries and their dependencies for click and click-completion.

thechief389 avatar Dec 02 '18 22:12 thechief389

Another argument against click. After spending 15 minutes I haven't found the code to implement help in this manner:

  1. <command> -h
  2. <command> --help
  3. help <command>
  4. help <command> -h
  5. help <command> --help
  6. help

There 4. 5. 6. are equivalent to help -h or help --help.

techtonik avatar Jan 15 '19 07:01 techtonik

Hello, I'm one of the Pallets (Click) core maintainers. I'm happy to answer questions you have, although I'd ask that you ping me here or in our Discord rather than opening up issues for questions in the repo.

I think we're already set up to be vendorable, and if not we can fix that. If you notice any bugs while trying to integrate Click with Pip, please let us know.

davidism avatar Jan 15 '19 14:01 davidism

The basic implementation of a help name command, although it should be improved:

@cli.command()
@click.argument("name")
@click.pass_context
def help(ctx, name):
    command = cli.get_command(ctx, name)
    click.echo(command.get_help(ctx))

davidism avatar Jan 15 '19 14:01 davidism

Thanks for pitching in here @davidsm! :)

I'll ping here if and when I work on this. :)

pradyunsg avatar Jan 15 '19 16:01 pradyunsg

Because of the size of the code base, I think it's important to have an implementation plan that would let us make progress on this incrementally.

cjerdonek avatar Jan 15 '19 17:01 cjerdonek

We can't really have half our parsing be optparse and the other half be click. I think you know that. ;) I think you mean what the plan is for separating concerns within pip's codebase about this to ease the actual "switch". My current thought process for that is:

  • modify pip's Commands to stop passing "options" to other internal objects (like to pip_version_check).
    • mypy annotations should help here, though I don't think they'll cover 100%.
  • create custom Option classes and adding code to convert them into concrete optparse variants would make life easier later on.
  • it should be not-too-difficult to create an optparse.Values-like object using click's higher-level classes to be passed to Commands

PS: it's past 1 am. Don't quote me on these. :)

pradyunsg avatar Jan 15 '19 20:01 pradyunsg

We can't really have half our parsing be optparse and the other half be click.

That's not what I was saying.. I was saying I think it's important for us to have a way to do it that lets us make changes to the code gradually over time. There can be many possible approaches.

For example, maybe there is a way to add a translation / compatibility layer to have whatever we're migrating to (argparse, click, or whatever) accept optparse options. That would let us switch the options one at a time from optparse to the new format.

The point is to avoid a massive PR or massive amounts of code getting turned on at once. The approach should let us incrementally add new code that is getting used as we add it.

cjerdonek avatar Jan 15 '19 20:01 cjerdonek

modify pip's Commands to stop passing "options" to other internal objects (like to pip_version_check). ... it should be not-too-difficult to create an optparse.Values-like object using click's higher-level classes to be passed to Commands

Since the options objects are just bags of properties, it seems like it would still work for them to be passed around as long as the interfaces of the objects being passed around remain the same (e.g. same attribute names, etc).

There could be other reasons to stop passing options objects around (e.g. to make dependencies more explicit or to reduce coupling), but it doesn't seem like it's something that has to be done.

cjerdonek avatar Jan 15 '19 21:01 cjerdonek

from our experiences in pytest im relatively confident to claim that a basic command setup for argparse for example is relatively simple to do,

i suspect it would have similar complexity for pip (which didn't exist when re we revamped argument parsing in pytest)

however pip has a major advantage over pytest - the api is not public, so experiments and mistakes cant hit reasonable users at the api level

i also believe it would be a great help to layer things nicely and decouple the internals from details of the command line parser

RonnyPfannschmidt avatar Jan 15 '19 21:01 RonnyPfannschmidt

i also believe it would be a great help to layer things nicely and decouple the internals from details of the command line parser

In other words, switching from optparse should be just a matter of swapping single cli file.

techtonik avatar Jan 17 '19 08:01 techtonik

I distance myself from that reprasing of different meaning as for the time being its not clear how things will actually be

Its entirely possible that the ROI of a partial refactoring followed by a change is better than going all the way

RonnyPfannschmidt avatar Jan 17 '19 10:01 RonnyPfannschmidt

The point is to avoid a massive PR or massive amounts of code getting turned on at once. The approach should let us incrementally add new code that is getting used as we add it.

This, I agree with. I pretty weary of such PRs now.

There could be other reasons to stop passing options objects around (e.g. to make dependencies more explicit or to reduce coupling), but it doesn't seem like it's something that has to be done.

Yea, this is unrelated to us switching over from optparse. To digress for a bit, it's still something we should do IMO, to make data dependencies clearer.


Thinking about this in a less sleepy state, I think it should be possible to do optparse -> click conversion at the "Command" level. Then, we can start swapping out cmdoptions and sub-command parsers with their click equivalents.

pradyunsg avatar Jan 17 '19 11:01 pradyunsg

@pradyunsg looks like you've already decided to go for click. I still don't see that this decorator approach leads to readable code. It doesn't give a good overview about the whole CLI interface, and couples the whole codebase to click.

techtonik avatar Jan 18 '19 10:01 techtonik

I already stated that I don't want to be using decorators: https://github.com/pypa/pip/issues/4659#issuecomment-441323813. Click has other abstractions that I'd prefer to use.

Unless we drop Python 2 support, I don't think we'll be able to use argparse due to the behavior associated with allow_abrev.

pradyunsg avatar Jan 18 '19 14:01 pradyunsg

Pipenv uses click and it works well.

thechief389 avatar Jan 18 '19 16:01 thechief389

Poetry looks better and uses cleo.

techtonik avatar Jan 21 '19 08:01 techtonik

It's pretty simple to use click's underlying classes to make a CLI like pip's. The more fun part is figuring out how the transition for pip's optparse based code to have click options and making commands get Values-like objects, as @cjerdonek pointed out.

Click to reveal code-dump
import inspect
import functools

import click

from .commands import get_commands


def call_with_context(func):
    return func(click.get_current_context())


def create_click_command_from_our_command(our_command):
    # Convert the options; this bit has to be figured out.
    params = [
        click.Option(opt.param_decls, **opt.kwargs) for opt in our_command.options
    ]

    # Construct the click command
    cmd = click.Command(
        our_command.name,
        short_help=inspect.getdoc(our_command),
        callback=functools.partial(call_with_context, our_command.run),
        params=params,
    )

    return cmd


def main():
    cli = click.Group()
    for our_command in get_commands():
        cli.add_command(create_click_command_from_our_command(our_command))

    cli.main(prog_name="pip")

pradyunsg avatar Jan 21 '19 11:01 pradyunsg

Click doesn't support custom formatting - https://github.com/pallets/click/issues/561

techtonik avatar Feb 10 '19 04:02 techtonik