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

Rework command handling v2 #721

Merged
merged 11 commits into from Dec 7, 2012

Conversation

Projects
None yet
5 participants
@gvalkov
Contributor

gvalkov commented Nov 10, 2012

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,

gvalkov added some commits Nov 10, 2012

remove global command instances
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.
make monkey-patching work again in tests/test_user_site.py
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.
Show outdated Hide outdated pip/req.py Outdated
@pnasrat

This comment has been minimized.

Show comment
Hide comment
@pnasrat

pnasrat Nov 12, 2012

Contributor

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.

Contributor

pnasrat commented Nov 12, 2012

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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Nov 15, 2012

Contributor

@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.

Contributor

qwcode commented Nov 15, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Nov 15, 2012

Contributor

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 :\

Contributor

gvalkov commented Nov 15, 2012

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

This comment has been minimized.

Show comment
Hide comment
@dholth

dholth Nov 15, 2012

Member

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

Member

dholth commented Nov 15, 2012

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

@gvalkov

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Nov 19, 2012

Contributor

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

Contributor

gvalkov commented Nov 19, 2012

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

Show outdated Hide outdated pip/commands/help.py Outdated
Show outdated Hide outdated pip/commands/__init__.py Outdated
Show outdated Hide outdated pip/commands/__init__.py Outdated
Show outdated Hide outdated pip/commands/__init__.py Outdated
Show outdated Hide outdated pip/__init__.py Outdated
@jezdez

This comment has been minimized.

Show comment
Hide comment
@jezdez

jezdez Dec 6, 2012

Contributor

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

Contributor

jezdez commented Dec 6, 2012

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

@jezdez jezdez referenced this pull request Dec 6, 2012

Closed

Add "list" pip command #675

gvalkov added some commits Dec 6, 2012

fix error output
`pip help <misspelled>` should return the same error message as `pip
<misspelled>`, namely:

  ERROR: unknown command "enstall" - maybe you meant "install"
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 7, 2012

Contributor

looks like you handled jannis' comments.

I want to merge this, but 2 more thoughts:

  1. 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?

  2. 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

Contributor

qwcode commented Dec 7, 2012

looks like you handled jannis' comments.

I want to merge this, but 2 more thoughts:

  1. 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?

  2. 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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Dec 7, 2012

Contributor

@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 :)

Contributor

gvalkov commented Dec 7, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@pnasrat

pnasrat Dec 7, 2012

Contributor

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.

Contributor

pnasrat commented Dec 7, 2012

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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 7, 2012

Contributor

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.

Contributor

qwcode commented Dec 7, 2012

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.

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

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

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 8, 2012

Contributor

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

Contributor

qwcode commented Dec 8, 2012

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

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

@gvalkov gvalkov referenced this pull request Dec 16, 2012

Closed

Rework command handling #463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment