Skip to content

Refactoring#428

Closed
gvalkov wants to merge 11 commits intopypa:developfrom
gvalkov:refactor
Closed

Refactoring#428
gvalkov wants to merge 11 commits intopypa:developfrom
gvalkov:refactor

Conversation

@gvalkov
Copy link
Contributor

@gvalkov gvalkov commented Jan 6, 2012

This branch attempts to address some areas of pip that I think need more work.

Look and Feel

old:
[pip help](https://gist.github.com/raw/1556718/d82231058dd3c3ff3e1529eaf325b4f5d3075540/pip%20help%20(develop%206d6ead)
[pip help install](https://gist.github.com/raw/1556718/76f4e7ded3301d9595a62196a80c9d66c665dcba/pip%20help%20install%20(develop%206d6ead)

new:
pip help
pip help install

  • Less verbose command line help (long option descriptions will be moved to a man page).
  • Prettier and more compact option formatting.
  • Group related options.
  • pip pip --help and pip help with no arguments should behave in the same way.
  • No need for --src=DIR, --source=DIR, --source-dir=DIR, --source-directory=DIR like lines. Decide on two and maintain compatibility for the rest.

Code

  • No global parser and commands.
  • No need for deferred command loading - deferred instantiation should be enough.
  • Better project structure.
  • Better configuration handling (instead of an optparse.Values)
  • Many more ...

... Listing all these would turn this into a design document and I actually prefer fixing things. Please refer to the code - the commits are fairly atomic (mostly for demonstrative purposes)

This is still mostly a work in progress, but since refactoring work often ends up being un-mergeable, I'd be glad if this was merged earlier.

gvalkov added 11 commits January 5, 2012 14:12
The pip.version module provides several useful functions for getting
pip's version. It is no longer necessary to change the version
seperately in docs/conf.py and setup.py.
Example:
	-E DIR, --environment=DIR -> -E, --environment <dir>
n.b: this commit breaks 'pip <command> --version' ; fix later
- The global 'parser' object was removed. At present it is instantiated
  in main() and passed on to each Command (stored as Command.main_parser)

- All global command instances were removed. Deferred command imports
  were removed. Commands are instantiated on demand in main(). See
  pip.commands.__init__

- The command list is now part of the parser's description and comes
  before all other options. The pip help command now serves the purpose
  of showing a given command's options instead of all available
  commands.

- Option handling was factored out of main()

- All pip exceptions inherit from PipError

- All command specific options grouped under a 'Command options'
  OptionGroup (very lame implementation at the moment)

- Changed the wording of some options (wip)
 - Reordering imports
 - ''' -> """
 - __all__ lists to __all__ tuples
 - Small consistency fixes
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of change is pure, useless noise in a diff. Please try to avoid such changes in pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have separated these into '[style]' commits (or done nothing at all actually, since such things annoy us all differently)

@carljm
Copy link
Contributor

carljm commented Jan 8, 2012

Hi @gvalkov - thanks very much for the work here. I'm excited about the improved help output, so I'd like to get this merged, but there are some significant issues remaining to be addressed before that can happen.

First, pip supports Python 2.4 through 3.2, all from a single codebase. This requires that in some cases we sacrifice beautiful code for this wider support. The worst is catching exceptions where we need the exception instance: since Python 3 requires the "as" syntax and Python < 2.6 doesn't support it, you must simply catch the exception with no as e or , e and then do e = sys.exc_info()[1] inside the except block. Also, no use of e.g. dict.iteritems which does not exist in Python 3, no use of "x if y else z" which isn't supported in older versions, always use parentheses with print...

Secondly, the existing tests must pass. I've tried to get the tests passing, and in doing so I fixed a number of Python version compatibility issues that I found, as well as some typos and unwanted/unnecessary changes, and pushed to https://github.com/carljm/pip/tree/gvalkov-refactor. I was still blocked on getting the tests to run on Python 2.4, though - it seems the way your code interacts with optparse is broken with Python 2.4's optparse. This will need to be fixed. On Python 3.2 I'm seeing at least one test failure, in an autocompletion test.

Lastly, I'm not sure why there are so many cosmetic rearrangements of things like docstrings, or replacing lists with tuples (incorrectly, IMO), in this pull request. I won't block the pull request on that kind of thing, since I'd really like to get these improvements in - but in future please keep your diffs minimal. Unless there's a really good reason for a cosmetic change, it just makes the diff harder to review and makes future use of "git annotate" more difficult.

Thanks again! Hopefully we can get this into merge shape soon.

@gvalkov
Copy link
Contributor Author

gvalkov commented Jan 8, 2012

Thanks @carljm - I replied to some of your code comments. Honeslty, I've never had to deal with future/backward compatibiliy to such an extend (2.4 to 3.2 is like ~7 years). Some of these things just don't pop up in my head (dict.iteritems was a stupid mistake, but e = sys.exc_info()[1] is something I didn't know - I though except Exception, e: was valid in 2.4)

The point of the pull request was to get more eyes on the code, since it changes things all over the place. I didn't consider it completely mergeable either. I'll definitely have a look at your fork and work on getting it into merge shape, as you said.

@gvalkov
Copy link
Contributor Author

gvalkov commented Feb 6, 2012

I'm closing this as I won't have any time to work on it in the foreseeable future. Thanks @carljm for the time spend reviewing and on #pip. Pip is such a flagship tool for anyone in the Python world and I really hope I can get back to hacking on it again.

@gvalkov gvalkov closed this Feb 6, 2012
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

auto-locked Outdated issues that have been locked by automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants