Rework command handling v2 by gvalkov · Pull Request #721 · pypa/pip

gvalkov · 2012-11-10T17:57:44Z

Hi, refactoring guy here again. I decided to resurrect my command line rework patch from a while ago (~9 months). Basically I think that pip's command line interface is not up to par with what you'd expect from Python's flagship package manager. It's quite possibly, the worst one of all package managers that fall in the same category (see gem, npm, luarocks, cabal), in terms of command line look & feel.

In pull requests #463 and #533, I proposed turning pip's cli from this to this. It's not a big improvement, but it's a start. Unfortunately, I just couldn't bring myself to implementing these without refactoring a few things first and while the general vibe about these changes was positive, very few of the them made it in.

This pull request contains just the refactoring portion of the previous two requests (no ui). To summarize:

Remove all global command instances and deferred command loading. What replaces them is a command 'registry' (really a mapping of command names to command classes). Commands are instantiated on demand in pip.__init__.main(). See pip.commands.__init__.
Remove baseparser.parser - the global OptionParser instance. Each Command instance now receives the main OptionParser as its first argument. The main OptionParser itself is created with baseparser.create_main_parser().
Clean up pip.__init__ by taking option parsing out of main().

Hopefully, this pull request is more 'reviewable' and 'mergeable' than the previous two. The interface changes themselves will come in another round of commits, probably right after I get some assurance that this will get pulled in. Pip has a long pull queue and while this may not be the most important pull of all, some refactoring and attention to detail is long due.

I hope I'm not coming out as arrogant, but I'm concerned about wasting any more time on this, given your track record of accepting my contributions :) I really just want Python to have an excellent package manager.

Cheers,

Rework the way commands are defined, loaded and ran in pip: - Commands are instantiated on demand in pip.main(). - A command 'registry' - mapping of command names to command classes in pip.commands.__init__. - Remove deferred command module loading.

Monkey-patching 'pip.util.dist_in_site_packages' through 'sitecustomize.py' in 'tests/test_user_site.py' wasn't working unless the fully qualitifed path to 'dist_in_site_packages' was used.

gvalkov · 2012-11-10T18:03:47Z

pip/req.py

This is a very unfortunate addition, but the monkey patching in tests/test_user_site.py wasn't working otherwise.

It'd be good to figure out a way around this.

hey @gvalkov , I'll try to create the user site test problems you were having and see if there's a workaround.

@gvalkov : see the pull I submitted to your fork on how to remove the import pip
gvalkov#1

pnasrat · 2012-11-12T12:50:42Z

We do appreciate your effort - but all the maintainers are volunteers and personally my time is pretty limited at the moment.

I'll do a first pass and leave comments.

Generally reviews will be easier if you do small incremental pull requests, it's easier to follow the refactoring and comment on - if you look at Issue #511 for an example for where we've done this before.

qwcode · 2012-11-15T04:52:13Z

@gvalkov , can you spell out for us why the 3 bullet pt refactors above are needed or preferred for what's coming?

I appreciate the links to the other pkg manager help outputs, and like I said before in a previous pull, I'm attracted to the new help format for pip you link to, that would follow this.

gvalkov · 2012-11-15T14:18:25Z

Certainly. Here goes:

The whole way of loading and instantiating commands seemed very convoluted. When a command is needed, its module is imported and the command class is instantiated in the imported module itself. At instantiation time, the command is added to the global command_dict from where it is subsequently used.

Now, there is nothing technically wrong with this way of doing things, it's just hard to follow and work with. My proposed solution is quite simple - get rid of the deferred command loading functionality (command_dict load_command load_all_commands command_names and all module level command instances) and replace it with a command 'registry'. Maybe there was once an argument for deferred loading, but now it seems like a premature optimization that does nothing but complicate things.

Having things structured this way allowed me to add the command names and summaries to the description of the main option parser. A semi-accidental ui change that got in with this is that running pip, pip help or pip --help all print the main parser's usage. Previously, running pip without any arguments or options would have given you the unhelpful You must give a command (use "pip help" to see a list of commands)

All in all, I just think that this change makes things more obvious and easier to hack on.
Removing the global baseparser.parser instance had to do with adding multiple OptionGroups. My idea is to differentiate between the options that come from baseparser (--help, --log, --quiet etc) and those that come from commands (--user, --update etc). This change adds the groundwork to make that happen. Each command is passed a reference to the main parser from which it copies all options and options groups (I admit, it's a bit messy).

In retrospect, it might have been easier and clearer to just bundle argparse with pip and use its subparsers instead of implementing your own on top of optparse. It would be great if there was a level of indirection between options/settings and their presentation (like it is in IPython). Maybe for pip 2.0 ;)
Taking option parsing out of pip.__init__.main is just for niceness and clarity. I like how main() ends up being this simple 15 line function. The changes are very straightforward, see for yourselves.

Hope this wall of text was helpful. Unfortunately, the changes are all very intertwined and I didn't see a way of breaking them down any further :\

dholth · 2012-11-15T14:28:29Z

pip is older than argparse (at least, older than PEP 389), so it's not surprising that it does not use it.

patch dist_in_site_packages for user_site tests

gvalkov · 2012-11-19T21:07:02Z

My bad, I should have --ff merged @qwcode's fix, instead of using github's interface.

jezdez · 2012-12-06T08:59:28Z

pip/commands/help.py

Please no bare exception handling.

jezdez · 2012-12-06T09:02:37Z

This looks good in general, but please fix the issues I've commented on before we merge this.

`pip help <misspelled>` should return the same error message as `pip <misspelled>`, namely: ERROR: unknown command "enstall" - maybe you meant "install"

qwcode · 2012-12-07T06:43:37Z

looks like you handled jannis' comments.

I want to merge this, but 2 more thoughts:

I know subsequent pulls are planned to improve help formatting, but I'd prefer not leave these uneven indents. Do you mind evening that up now?
since you admitted copying the main parser's options was messy, let me offer another idea. I don't like the copy routine either. see this commit . Don't take the idea verbatim, but the gist is to make the option objects reusable from multiple routines (that would handle adding them to groups when needed/desired), w/o passing the base parser object. I'm doing something similar in my wheel fork to prevent redefining shared options

gvalkov · 2012-12-07T19:43:41Z

@qwcode, I've aligned the command and option lists, but I'm not sure how to go about passing the main parser to individual commands. My 30 minute go at it today resulted in some serious build breakage - will look into it these days. At this point I feel like porting all option handling to argparse, instead of having to deal with the current implementation (It really isn't such a crazy idea :)

pnasrat · 2012-12-07T20:08:18Z

We need to support 2.5 and 2.6, I guess we could depend on http://code.google.com/p/argparse/

I'd say if all the minor nits are done lets get this in and iterate on it so we can start improving the commands.

qwcode · 2012-12-07T22:43:51Z

sure, let's merge with this, and iterate more in the next pull.

I confirmed the indent fixes.

but @gvalkov , see my rework-commands-v2_options branch.
no test breakage with the changes I'm talking about.

I'm hesitant to add a dependency on argparse, since it means a change to virtualenv.

Rework command handling v2

qwcode · 2012-12-08T17:57:43Z

@gvalkov just noticed you're not in the authors file. I'll add you.

gvalkov added 5 commits November 10, 2012 15:30

remove global command instances

7427eca

Rework the way commands are defined, loaded and ran in pip: - Commands are instantiated on demand in pip.main(). - A command 'registry' - mapping of command names to command classes in pip.commands.__init__. - Remove deferred command module loading.

add create_main_parser() and the '-V --version' option

8dbdf50

add the base_parser argument to all Command instances

46a2175

replace global OptionParser and refactor main()

2c64a1e

make monkey-patching work again in tests/test_user_site.py

7746051

Monkey-patching 'pip.util.dist_in_site_packages' through 'sitecustomize.py' in 'tests/test_user_site.py' wasn't working unless the fully qualitifed path to 'dist_in_site_packages' was used.

gvalkov reviewed Nov 10, 2012
View reviewed changes

qwcode and others added 2 commits November 19, 2012 09:38

patch dist_in_site_packages for user_site tests

4c8ac16

Merge pull request #1 from qwcode/rework-commands-v2-qwcode

b4fe4ff

patch dist_in_site_packages for user_site tests

qwcode mentioned this pull request Nov 24, 2012

Make -U non-recursive by default, add --upgrade-recursive option #571

Closed

jezdez reviewed Dec 6, 2012
View reviewed changes

pip/commands/help.py Outdated

Copy link

Member

jezdez Dec 6, 2012

Choose a reason for hiding this comment

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

Please no bare exception handling.

jezdez mentioned this pull request Dec 6, 2012

Add "list" pip command #675

Closed

gvalkov added 3 commits December 6, 2012 17:25

pep8 fixes

7f4503b

fix bare except statement

e103e54

fix error output

715c7ef

`pip help <misspelled>` should return the same error message as `pip <misspelled>`, namely: ERROR: unknown command "enstall" - maybe you meant "install"

align command list with options

2559c78

qwcode added a commit that referenced this pull request Dec 7, 2012

Merge pull request #721 from gvalkov/rework-commands-v2

3a8f69b

Rework command handling v2

qwcode merged commit 3a8f69b into pypa:develop Dec 7, 2012

qwcode added a commit that referenced this pull request Dec 8, 2012

update authors file for pull #721

1976a65

gvalkov mentioned this pull request Dec 16, 2012

Rework command handling #463

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019

Conversation

gvalkov commented Nov 10, 2012

Uh oh!

gvalkov Nov 10, 2012

Choose a reason for hiding this comment

Uh oh!

pnasrat Nov 12, 2012

Choose a reason for hiding this comment

Uh oh!

qwcode Nov 15, 2012

Choose a reason for hiding this comment

Uh oh!

qwcode Nov 19, 2012

Choose a reason for hiding this comment

Uh oh!

pnasrat commented Nov 12, 2012

Uh oh!

qwcode commented Nov 15, 2012

Uh oh!

gvalkov commented Nov 15, 2012

Uh oh!

dholth commented Nov 15, 2012

Uh oh!

gvalkov commented Nov 19, 2012

Uh oh!

jezdez Dec 6, 2012

Choose a reason for hiding this comment

Uh oh!

jezdez commented Dec 6, 2012

Uh oh!

qwcode commented Dec 7, 2012

Uh oh!

gvalkov commented Dec 7, 2012

Uh oh!

pnasrat commented Dec 7, 2012

Uh oh!

qwcode commented Dec 7, 2012

Uh oh!

qwcode commented Dec 8, 2012

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants