From c6495a46e37482d2b01872538f0a64f41d277149 Mon Sep 17 00:00:00 2001 From: Georgi Valkov Date: Sat, 9 Mar 2013 15:33:36 +0200 Subject: [PATCH 1/6] remove basecommand.merge_options() This patch removes the error prone step of merging the baseparsers' options (the 'Generic Options') with those of the subparser. In fact, commands are already capable of parsing all options present in the 'Generic Options' option group. The option parsing logic is as follows: Given a command line such as: '--timeout 5 install --user pkg' pip.parseopt() returns command 'install' with arguments: '--timeout 5 --user pkg' pip.main() executes the above as: commands['install'](parser).main('--timeout 5 --user pkg', options) --- pip/__init__.py | 19 ++++++++++++------- pip/basecommand.py | 14 -------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/pip/__init__.py b/pip/__init__.py index 216268d6be0..b4778b0d56d 100755 --- a/pip/__init__.py +++ b/pip/__init__.py @@ -100,7 +100,10 @@ def parseopts(args): description = [''] + ['%-27s %s' % (i, j) for i, j in command_summaries] parser.description = '\n'.join(description) - options, args = parser.parse_args(args) + options, cmd_args = parser.parse_args(args) + # args: ['--timeout=5', 'install', '--user', 'INITools'] + # cmd_args: ['install', '--user', 'INITools'] + # note: parser calls disable_interspersed_args() if options.version: sys.stdout.write(parser.version) @@ -108,16 +111,18 @@ def parseopts(args): sys.exit() # pip || pip help || pip --help -> print_help() - if not args or (args[0] == 'help' and len(args) == 1): + if not cmd_args or (cmd_args[0] == 'help' and len(cmd_args) == 1): parser.print_help() sys.exit() - if not args: + if not cmd_args: msg = ('You must give a command ' '(use "pip --help" to see a list of commands)') raise CommandError(msg) - command = args[0].lower() + command = cmd_args[0].lower() # install + args.remove(cmd_args[0]) + cmd_args = args # --timeout=5 --user INITools if command not in commands: guess = get_similar_commands(command) @@ -128,7 +133,7 @@ def parseopts(args): raise CommandError(' - '.join(msg)) - return command, options, args, parser + return command, options, cmd_args, parser def main(initial_args=None): @@ -138,7 +143,7 @@ def main(initial_args=None): autocomplete() try: - cmd_name, options, args, parser = parseopts(initial_args) + cmd_name, options, cmd_args, parser = parseopts(initial_args) except PipError: e = sys.exc_info()[1] sys.stderr.write("ERROR: %s" % e) @@ -146,7 +151,7 @@ def main(initial_args=None): sys.exit(1) command = commands[cmd_name](parser) # see baseparser.Command - return command.main(args[1:], options) + return command.main(cmd_args, options) def bootstrap(): diff --git a/pip/basecommand.py b/pip/basecommand.py index 677bda98efc..eb4e525c5c0 100644 --- a/pip/basecommand.py +++ b/pip/basecommand.py @@ -68,25 +68,11 @@ def _copy_option_group(self, parser, group): parser.add_option_group(new_group) - def merge_options(self, initial_options, options): - # Make sure we have all global options carried over - attrs = ['log', 'proxy', 'require_venv', - 'log_explicit_levels', 'log_file', - 'timeout', 'default_vcs', - 'skip_requirements_regex', - 'no_input', 'exists_action', - 'cert'] - for attr in attrs: - setattr(options, attr, getattr(initial_options, attr) or getattr(options, attr)) - options.quiet += initial_options.quiet - options.verbose += initial_options.verbose - def setup_logging(self): pass def main(self, args, initial_options): options, args = self.parser.parse_args(args) - self.merge_options(initial_options, options) level = 1 # Notify level += options.verbose From 5245e62bcf2f6103d932237a202267ae1fae5404 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Mon, 9 Sep 2013 20:56:46 -0700 Subject: [PATCH 2/6] don't pass around options we're not using anymore --- pip/__init__.py | 8 ++++---- pip/basecommand.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pip/__init__.py b/pip/__init__.py index b4778b0d56d..add18877b0b 100755 --- a/pip/__init__.py +++ b/pip/__init__.py @@ -121,7 +121,7 @@ def parseopts(args): raise CommandError(msg) command = cmd_args[0].lower() # install - args.remove(cmd_args[0]) + args.remove(cmd_args[0]) cmd_args = args # --timeout=5 --user INITools if command not in commands: @@ -133,7 +133,7 @@ def parseopts(args): raise CommandError(' - '.join(msg)) - return command, options, cmd_args, parser + return command, cmd_args, parser def main(initial_args=None): @@ -143,7 +143,7 @@ def main(initial_args=None): autocomplete() try: - cmd_name, options, cmd_args, parser = parseopts(initial_args) + cmd_name, cmd_args, parser = parseopts(initial_args) except PipError: e = sys.exc_info()[1] sys.stderr.write("ERROR: %s" % e) @@ -151,7 +151,7 @@ def main(initial_args=None): sys.exit(1) command = commands[cmd_name](parser) # see baseparser.Command - return command.main(cmd_args, options) + return command.main(cmd_args) def bootstrap(): diff --git a/pip/basecommand.py b/pip/basecommand.py index eb4e525c5c0..3b4c7d824a2 100644 --- a/pip/basecommand.py +++ b/pip/basecommand.py @@ -71,7 +71,7 @@ def _copy_option_group(self, parser, group): def setup_logging(self): pass - def main(self, args, initial_options): + def main(self, args): options, args = self.parser.parse_args(args) level = 1 # Notify From d21d0e434ea86d8cde08f78e15eaf582a53a039e Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Tue, 10 Sep 2013 17:55:08 -0700 Subject: [PATCH 3/6] 1) have `create_main_parser` construct the parser description. 2) have `create_main_parser` mark the parser as the 'main' parser. 3) remove an extraneus block that's never called. thanks @netspyer. 4) move 'create_main_parser` to __init__ to avoid circular or embedded imports. --- pip/__init__.py | 76 ++++++++++++++++++++++----------- pip/baseparser.py | 23 ---------- tests/functional/test_help.py | 2 +- tests/functional/test_search.py | 2 +- 4 files changed, 53 insertions(+), 50 deletions(-) diff --git a/pip/__init__.py b/pip/__init__.py index add18877b0b..adb12580cf5 100755 --- a/pip/__init__.py +++ b/pip/__init__.py @@ -9,9 +9,9 @@ from pip.log import logger from pip.util import get_installed_distributions, get_prog from pip.vcs import git, mercurial, subversion, bazaar # noqa -from pip.baseparser import create_main_parser -from pip.commands import commands, get_similar_commands, get_summaries - +from pip.baseparser import (ConfigOptionParser, UpdatingDefaultsHelpFormatter, + version, standard_options) +from pip.commands import commands, get_summaries, get_similar_commands # The version as used in the setup.py and the docs conf.py __version__ = "1.5.dev1" @@ -90,50 +90,76 @@ def autocomplete(): sys.exit(1) -def parseopts(args): - parser = create_main_parser() +def create_main_parser(): + parser_kw = { + 'usage': '\n%prog [options]', + 'add_help_option': False, + 'formatter': UpdatingDefaultsHelpFormatter(), + 'name': 'global', + 'prog': get_prog(), + } + + parser = ConfigOptionParser(**parser_kw) + genopt = optparse.OptionGroup(parser, 'General Options') + parser.disable_interspersed_args() + + # having a default version action just causes trouble + parser.version = version + + for opt in standard_options: + genopt.add_option(opt) + parser.add_option_group(genopt) + parser.main = True # so the help formatter knows - # create command listing + # create command listing for description command_summaries = get_summaries() - description = [''] + ['%-27s %s' % (i, j) for i, j in command_summaries] parser.description = '\n'.join(description) - options, cmd_args = parser.parse_args(args) - # args: ['--timeout=5', 'install', '--user', 'INITools'] - # cmd_args: ['install', '--user', 'INITools'] - # note: parser calls disable_interspersed_args() + return parser + + +def parseopts(args): + parser = create_main_parser() - if options.version: + # Note: parser calls disable_interspersed_args(), so the result of this call + # is to split the initial args into the general options before the + # subcommand and everything else. + # For example: + # args: ['--timeout=5', 'install', '--user', 'INITools'] + # general_options: ['--timeout==5'] + # args_else: ['install', '--user', 'INITools'] + general_options, args_else = parser.parse_args(args) + + # --version + if general_options.version: sys.stdout.write(parser.version) sys.stdout.write(os.linesep) sys.exit() - # pip || pip help || pip --help -> print_help() - if not cmd_args or (cmd_args[0] == 'help' and len(cmd_args) == 1): + # pip || pip help -> print_help() + if not args_else or (args_else[0] == 'help' and len(args_else) == 1): parser.print_help() sys.exit() - if not cmd_args: - msg = ('You must give a command ' - '(use "pip --help" to see a list of commands)') - raise CommandError(msg) + # the subcommand name + cmd_name = args_else[0].lower() - command = cmd_args[0].lower() # install - args.remove(cmd_args[0]) - cmd_args = args # --timeout=5 --user INITools + #all the args without the subcommand + cmd_args = args[:] + cmd_args.remove(args_else[0].lower()) - if command not in commands: - guess = get_similar_commands(command) + if cmd_name not in commands: + guess = get_similar_commands(cmd_name) - msg = ['unknown command "%s"' % command] + msg = ['unknown command "%s"' % cmd_name] if guess: msg.append('maybe you meant "%s"' % guess) raise CommandError(' - '.join(msg)) - return command, cmd_args, parser + return cmd_name, cmd_args, parser def main(initial_args=None): diff --git a/pip/baseparser.py b/pip/baseparser.py index bb232c7e1fa..4c1dc907939 100644 --- a/pip/baseparser.py +++ b/pip/baseparser.py @@ -229,29 +229,6 @@ def error(self, msg): version = None -def create_main_parser(): - parser_kw = { - 'usage': '\n%prog [options]', - 'add_help_option': False, - 'formatter': UpdatingDefaultsHelpFormatter(), - 'name': 'global', - 'prog': get_prog(), - } - - parser = ConfigOptionParser(**parser_kw) - genopt = optparse.OptionGroup(parser, 'General Options') - parser.disable_interspersed_args() - - # having a default version action just causes trouble - parser.version = version - - for opt in standard_options: - genopt.add_option(opt) - parser.add_option_group(genopt) - - return parser - - standard_options = [ optparse.make_option( '-h', '--help', diff --git a/tests/functional/test_help.py b/tests/functional/test_help.py index e5202fb4f60..3d32db706c1 100644 --- a/tests/functional/test_help.py +++ b/tests/functional/test_help.py @@ -1,7 +1,7 @@ import pytest from pip.exceptions import CommandError -from pip.baseparser import create_main_parser +from pip import create_main_parser from pip.basecommand import ERROR, SUCCESS from pip.commands.help import HelpCommand from pip.commands import commands diff --git a/tests/functional/test_search.py b/tests/functional/test_search.py index 04cc34e18db..6e6e5e7bcc8 100644 --- a/tests/functional/test_search.py +++ b/tests/functional/test_search.py @@ -5,7 +5,7 @@ SearchCommand) from pip.status_codes import NO_MATCHES_FOUND, SUCCESS from pip.backwardcompat import xmlrpclib, b -from pip.baseparser import create_main_parser +from pip import create_main_parser from mock import Mock from tests.lib import pyversion From 15910f573e5b5b35b603806ccab5b7dddfb36c3a Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Mon, 16 Sep 2013 22:21:15 -0700 Subject: [PATCH 4/6] define options once, but don't globally instantiate --- docs/pipext.py | 7 +- pip/__init__.py | 18 +-- pip/basecommand.py | 33 ++---- pip/baseparser.py | 121 +------------------ pip/cmdoptions.py | 201 ++++++++++++++++++++++++++++---- pip/commands/help.py | 2 +- pip/commands/install.py | 16 +-- pip/commands/wheel.py | 12 +- tests/functional/test_help.py | 7 +- tests/functional/test_search.py | 5 +- 10 files changed, 222 insertions(+), 200 deletions(-) diff --git a/docs/pipext.py b/docs/pipext.py index f2902ed0aa3..747b314346e 100644 --- a/docs/pipext.py +++ b/docs/pipext.py @@ -7,7 +7,6 @@ from docutils.statemachine import ViewList from textwrap import dedent from pip import commands -from pip.baseparser import create_main_parser, standard_options from pip import cmdoptions from pip.util import get_prog @@ -79,19 +78,19 @@ def run(self): class PipGeneralOptions(PipOptions): def process_options(self): - self._format_options(standard_options) + self._format_options([o.make() for o in cmdoptions.general_group['options']]) class PipIndexOptions(PipOptions): def process_options(self): - self._format_options(cmdoptions.index_group['options']) + self._format_options([o.make() for o in cmdoptions.index_group['options']]) class PipCommandOptions(PipOptions): required_arguments = 1 def process_options(self): - cmd = commands[self.arguments[0]](create_main_parser()) + cmd = commands[self.arguments[0]]() self._format_options(cmd.parser.option_groups[0].option_list, cmd_name=cmd.name) diff --git a/pip/__init__.py b/pip/__init__.py index adb12580cf5..f0dd91c755e 100755 --- a/pip/__init__.py +++ b/pip/__init__.py @@ -5,12 +5,13 @@ import sys import re +from pip import cmdoptions from pip.exceptions import InstallationError, CommandError, PipError from pip.log import logger from pip.util import get_installed_distributions, get_prog from pip.vcs import git, mercurial, subversion, bazaar # noqa from pip.baseparser import (ConfigOptionParser, UpdatingDefaultsHelpFormatter, - version, standard_options) + version) from pip.commands import commands, get_summaries, get_similar_commands # The version as used in the setup.py and the docs conf.py @@ -60,7 +61,7 @@ def autocomplete(): print(dist) sys.exit(1) - subcommand = commands[subcommand_name](parser) + subcommand = commands[subcommand_name]() options += [(opt.get_opt_string(), opt.nargs) for opt in subcommand.parser.option_list_all if opt.help != optparse.SUPPRESS_HELP] @@ -100,15 +101,14 @@ def create_main_parser(): } parser = ConfigOptionParser(**parser_kw) - genopt = optparse.OptionGroup(parser, 'General Options') parser.disable_interspersed_args() # having a default version action just causes trouble parser.version = version - for opt in standard_options: - genopt.add_option(opt) - parser.add_option_group(genopt) + # add the general options + gen_opts = cmdoptions.make_option_group(cmdoptions.general_group, parser) + parser.add_option_group(gen_opts) parser.main = True # so the help formatter knows @@ -159,7 +159,7 @@ def parseopts(args): raise CommandError(' - '.join(msg)) - return cmd_name, cmd_args, parser + return cmd_name, cmd_args def main(initial_args=None): @@ -169,14 +169,14 @@ def main(initial_args=None): autocomplete() try: - cmd_name, cmd_args, parser = parseopts(initial_args) + cmd_name, cmd_args = parseopts(initial_args) except PipError: e = sys.exc_info()[1] sys.stderr.write("ERROR: %s" % e) sys.stderr.write(os.linesep) sys.exit(1) - command = commands[cmd_name](parser) # see baseparser.Command + command = commands[cmd_name]() return command.main(cmd_args) diff --git a/pip/basecommand.py b/pip/basecommand.py index 3b4c7d824a2..3c412cfb75c 100644 --- a/pip/basecommand.py +++ b/pip/basecommand.py @@ -8,6 +8,7 @@ import time import optparse +from pip import cmdoptions from pip.log import logger from pip.download import urlopen from pip.exceptions import (BadCommand, InstallationError, UninstallationError, @@ -31,7 +32,7 @@ class Command(object): usage = None hidden = False - def __init__(self, main_parser): + def __init__(self): parser_kw = { 'usage': self.usage, 'prog': '%s %s' % (get_prog(), self.name), @@ -40,39 +41,27 @@ def __init__(self, main_parser): 'name': self.name, 'description': self.__doc__, } - self.main_parser = main_parser + self.parser = ConfigOptionParser(**parser_kw) # Commands should add options to this option group optgroup_name = '%s Options' % self.name.capitalize() self.cmd_opts = optparse.OptionGroup(self.parser, optgroup_name) - # Re-add all options and option groups. - for group in main_parser.option_groups: - self._copy_option_group(self.parser, group) - - # Copies all general options from the main parser. - self._copy_options(self.parser, main_parser.option_list) - - def _copy_options(self, parser, options): - """Populate an option parser or group with options.""" - for option in options: - if not option.dest: - continue - parser.add_option(option) + # Add the general options + gen_opts = cmdoptions.make_option_group(cmdoptions.general_group, self.parser) + self.parser.add_option_group(gen_opts) - def _copy_option_group(self, parser, group): - """Copy option group (including options) to another parser.""" - new_group = optparse.OptionGroup(parser, group.title) - self._copy_options(new_group, group.option_list) - - parser.add_option_group(new_group) def setup_logging(self): pass + def parse_args(self, args): + # factored out for testability + return self.parser.parse_args(args) + def main(self, args): - options, args = self.parser.parse_args(args) + options, args = self.parse_args(args) level = 1 # Notify level += options.verbose diff --git a/pip/baseparser.py b/pip/baseparser.py index 4c1dc907939..40f0610dda5 100644 --- a/pip/baseparser.py +++ b/pip/baseparser.py @@ -7,7 +7,7 @@ import textwrap from distutils.util import strtobool from pip.backwardcompat import ConfigParser, string_types -from pip.locations import default_config_file, default_log_file +from pip.locations import default_config_file from pip.util import get_terminal_size, get_prog @@ -227,122 +227,3 @@ def error(self, msg): except pkg_resources.DistributionNotFound: # when running pip.py without installing version = None - - -standard_options = [ - optparse.make_option( - '-h', '--help', - dest='help', - action='help', - help='Show help.'), - - optparse.make_option( - # Run only if inside a virtualenv, bail if not. - '--require-virtualenv', '--require-venv', - dest='require_venv', - action='store_true', - default=False, - help=optparse.SUPPRESS_HELP), - - optparse.make_option( - '-v', '--verbose', - dest='verbose', - action='count', - default=0, - help='Give more output. Option is additive, and can be used up to 3 times.'), - - optparse.make_option( - '-V', '--version', - dest='version', - action='store_true', - help='Show version and exit.'), - - optparse.make_option( - '-q', '--quiet', - dest='quiet', - action='count', - default=0, - help='Give less output.'), - - optparse.make_option( - '--log', - dest='log', - metavar='file', - help='Log file where a complete (maximum verbosity) record will be kept.'), - - optparse.make_option( - # Writes the log levels explicitely to the log' - '--log-explicit-levels', - dest='log_explicit_levels', - action='store_true', - default=False, - help=optparse.SUPPRESS_HELP), - - optparse.make_option( - # The default log file - '--local-log', '--log-file', - dest='log_file', - metavar='file', - default=default_log_file, - help=optparse.SUPPRESS_HELP), - - optparse.make_option( - # Don't ask for input - '--no-input', - dest='no_input', - action='store_true', - default=False, - help=optparse.SUPPRESS_HELP), - - optparse.make_option( - '--proxy', - dest='proxy', - type='str', - default='', - help="Specify a proxy in the form [user:passwd@]proxy.server:port."), - - optparse.make_option( - '--timeout', '--default-timeout', - metavar='sec', - dest='timeout', - type='float', - default=15, - help='Set the socket timeout (default %default seconds).'), - - optparse.make_option( - # The default version control system for editables, e.g. 'svn' - '--default-vcs', - dest='default_vcs', - type='str', - default='', - help=optparse.SUPPRESS_HELP), - - optparse.make_option( - # A regex to be used to skip requirements - '--skip-requirements-regex', - dest='skip_requirements_regex', - type='str', - default='', - help=optparse.SUPPRESS_HELP), - - optparse.make_option( - # Option when path already exist - '--exists-action', - dest='exists_action', - type='choice', - choices=['s', 'i', 'w', 'b'], - default=[], - action='append', - metavar='action', - help="Default action when a path already exists: " - "(s)witch, (i)gnore, (w)ipe, (b)ackup."), - - optparse.make_option( - '--cert', - dest='cert', - type='str', - default='', - metavar='path', - help = "Path to alternate CA bundle."), - - ] diff --git a/pip/cmdoptions.py b/pip/cmdoptions.py index c6904dadda1..8f731a89a47 100644 --- a/pip/cmdoptions.py +++ b/pip/cmdoptions.py @@ -1,6 +1,15 @@ -"""shared options and groups""" -from optparse import make_option, OptionGroup, SUPPRESS_HELP -from pip.locations import build_prefix +""" +shared options and groups + +The principle here is to define options once, but *not* instantiate them globally. +One reason being that options with action='append' can carry state between parses. +pip parse's general options twice internally, and shouldn't pass on state. +To be consistent, all options will follow this design. + +""" +import copy +from optparse import OptionGroup, SUPPRESS_HELP, Option +from pip.locations import build_prefix, default_log_file def make_option_group(group, parser): @@ -11,21 +20,146 @@ def make_option_group(group, parser): """ option_group = OptionGroup(parser, group['name']) for option in group['options']: - option_group.add_option(option) + option_group.add_option(option.make()) return option_group +def make_option_maker(*args, **kwargs): + """Return an object with 'make()' method for creating the option later""" + class OptionMaker(): + @staticmethod + def make(): + args_copy = copy.deepcopy(args) + kwargs_copy = copy.deepcopy(kwargs) + return Option(*args_copy, **kwargs_copy) + return OptionMaker + ########### # options # ########### -index_url = make_option( +help_ = make_option_maker( + '-h', '--help', + dest='help', + action='help', + help='Show help.') + +require_virtualenv = make_option_maker( + # Run only if inside a virtualenv, bail if not. + '--require-virtualenv', '--require-venv', + dest='require_venv', + action='store_true', + default=False, + help=SUPPRESS_HELP) + +verbose = make_option_maker( + '-v', '--verbose', + dest='verbose', + action='count', + default=0, + help='Give more output. Option is additive, and can be used up to 3 times.') + +version = make_option_maker( + '-V', '--version', + dest='version', + action='store_true', + help='Show version and exit.') + +quiet = make_option_maker( + '-q', '--quiet', + dest='quiet', + action='count', + default=0, + help='Give less output.') + +log = make_option_maker( + '--log', + dest='log', + metavar='file', + help='Log file where a complete (maximum verbosity) record will be kept.') + +log_explicit_levels = make_option_maker( + # Writes the log levels explicitely to the log' + '--log-explicit-levels', + dest='log_explicit_levels', + action='store_true', + default=False, + help=SUPPRESS_HELP) + +log_file = make_option_maker( + # The default log file + '--local-log', '--log-file', + dest='log_file', + metavar='file', + default=default_log_file, + help=SUPPRESS_HELP) + +no_input = make_option_maker( + # Don't ask for input + '--no-input', + dest='no_input', + action='store_true', + default=False, + help=SUPPRESS_HELP) + +proxy = make_option_maker( + '--proxy', + dest='proxy', + type='str', + default='', + help="Specify a proxy in the form [user:passwd@]proxy.server:port.") + +timeout = make_option_maker( + '--timeout', '--default-timeout', + metavar='sec', + dest='timeout', + type='float', + default=15, + help='Set the socket timeout (default %default seconds).') + +default_vcs = make_option_maker( + # The default version control system for editables, e.g. 'svn' + '--default-vcs', + dest='default_vcs', + type='str', + default='', + help=SUPPRESS_HELP) + +skip_requirements_regex = make_option_maker( + # A regex to be used to skip requirements + '--skip-requirements-regex', + dest='skip_requirements_regex', + type='str', + default='', + help=SUPPRESS_HELP) + +exists_action = make_option_maker( + # Option when path already exist + '--exists-action', + dest='exists_action', + type='choice', + choices=['s', 'i', 'w', 'b'], + default=[], + action='append', + metavar='action', + help="Default action when a path already exists: " + "(s)witch, (i)gnore, (w)ipe, (b)ackup.") + +cert = make_option_maker( + '--cert', + dest='cert', + type='str', + default='', + metavar='path', + help = "Path to alternate CA bundle.") + +index_url = make_option_maker( '-i', '--index-url', '--pypi-url', dest='index_url', metavar='URL', default='https://pypi.python.org/simple/', help='Base URL of Python Package Index (default %default).') -extra_index_url = make_option( +extra_index_url = make_option_maker( '--extra-index-url', dest='extra_index_urls', metavar='URL', @@ -33,14 +167,14 @@ def make_option_group(group, parser): default=[], help='Extra URLs of package indexes to use in addition to --index-url.') -no_index = make_option( +no_index = make_option_maker( '--no-index', dest='no_index', action='store_true', default=False, help='Ignore package index (only looking at --find-links URLs instead).') -find_links = make_option( +find_links = make_option_maker( '-f', '--find-links', dest='find_links', action='append', @@ -49,7 +183,7 @@ def make_option_group(group, parser): help="If a url or path to an html file, then parse for links to archives. If a local path or file:// url that's a directory, then look for archives in the directory listing.") # TODO: Remove after 1.6 -use_mirrors = make_option( +use_mirrors = make_option_maker( '-M', '--use-mirrors', dest='use_mirrors', action='store_true', @@ -57,7 +191,7 @@ def make_option_group(group, parser): help=SUPPRESS_HELP) # TODO: Remove after 1.6 -mirrors = make_option( +mirrors = make_option_maker( '--mirrors', dest='mirrors', metavar='URL', @@ -65,7 +199,7 @@ def make_option_group(group, parser): default=[], help=SUPPRESS_HELP) -allow_external = make_option( +allow_external = make_option_maker( "--allow-external", dest="allow_external", action="append", @@ -74,7 +208,7 @@ def make_option_group(group, parser): help="Allow the installation of externally hosted files", ) -allow_all_external = make_option( +allow_all_external = make_option_maker( "--allow-all-external", dest="allow_all_external", action="store_true", @@ -82,7 +216,7 @@ def make_option_group(group, parser): help="Allow the installation of all externally hosted files", ) -no_allow_external = make_option( +no_allow_external = make_option_maker( "--no-allow-external", dest="allow_all_external", action="store_false", @@ -90,7 +224,7 @@ def make_option_group(group, parser): help=SUPPRESS_HELP, ) -allow_unsafe = make_option( +allow_unsafe = make_option_maker( "--allow-insecure", dest="allow_insecure", action="append", @@ -99,7 +233,7 @@ def make_option_group(group, parser): help="Allow the installation of insecure and unverifiable files", ) -no_allow_unsafe = make_option( +no_allow_unsafe = make_option_maker( "--no-allow-insecure", dest="allow_all_insecure", action="store_false", @@ -107,7 +241,7 @@ def make_option_group(group, parser): help=SUPPRESS_HELP ) -requirements = make_option( +requirements = make_option_maker( '-r', '--requirement', dest='requirements', action='append', @@ -116,27 +250,27 @@ def make_option_group(group, parser): help='Install from the given requirements file. ' 'This option can be used multiple times.') -use_wheel = make_option( +use_wheel = make_option_maker( '--use-wheel', dest='use_wheel', action='store_true', help='Find and prefer wheel archives when searching indexes and find-links locations. Default to accepting source archives.') -download_cache = make_option( +download_cache = make_option_maker( '--download-cache', dest='download_cache', metavar='dir', default=None, help='Cache downloaded packages in .') -no_deps = make_option( +no_deps = make_option_maker( '--no-deps', '--no-dependencies', dest='ignore_dependencies', action='store_true', default=False, help="Don't install package dependencies.") -build_dir = make_option( +build_dir = make_option_maker( '-b', '--build', '--build-dir', '--build-directory', dest='build_dir', metavar='dir', @@ -145,7 +279,7 @@ def make_option_group(group, parser): 'The default in a virtualenv is "/build". ' 'The default for global installs is "/pip_build_".') -install_options = make_option( +install_options = make_option_maker( '--install-option', dest='install_options', action='append', @@ -155,7 +289,7 @@ def make_option_group(group, parser): "Use multiple --install-option options to pass multiple options to setup.py install. " "If you are using an option with a directory path, be sure to use absolute path.") -global_options = make_option( +global_options = make_option_maker( '--global-option', dest='global_options', action='append', @@ -163,7 +297,7 @@ def make_option_group(group, parser): help="Extra global options to be supplied to the setup.py " "call before the install command.") -no_clean = make_option( +no_clean = make_option_maker( '--no-clean', action='store_true', default=False, @@ -174,6 +308,27 @@ def make_option_group(group, parser): # groups # ########## +general_group = { + 'name': 'General Options', + 'options': [ + help_, + require_virtualenv, + verbose, + version, + quiet, + log, + log_explicit_levels, + log_file, + no_input, + proxy, + timeout, + default_vcs, + skip_requirements_regex, + exists_action, + cert, + ] + } + index_group = { 'name': 'Package Index Options', 'options': [ diff --git a/pip/commands/help.py b/pip/commands/help.py index 4771db7539b..22533873224 100644 --- a/pip/commands/help.py +++ b/pip/commands/help.py @@ -27,7 +27,7 @@ def run(self, options, args): raise CommandError(' - '.join(msg)) - command = commands[cmd_name](self.main_parser) # instantiate + command = commands[cmd_name]() command.parser.print_help() return SUCCESS diff --git a/pip/commands/install.py b/pip/commands/install.py index 11099c5a228..99f686b7944 100644 --- a/pip/commands/install.py +++ b/pip/commands/install.py @@ -51,8 +51,8 @@ def __init__(self, *args, **kw): metavar='path/url', help='Install a project in editable mode (i.e. setuptools "develop mode") from a local project path or a VCS url.') - cmd_opts.add_option(cmdoptions.requirements) - cmd_opts.add_option(cmdoptions.build_dir) + cmd_opts.add_option(cmdoptions.requirements.make()) + cmd_opts.add_option(cmdoptions.build_dir.make()) cmd_opts.add_option( '-t', '--target', @@ -68,7 +68,7 @@ def __init__(self, *args, **kw): default=None, help="Download packages into instead of installing them, regardless of what's already installed.") - cmd_opts.add_option(cmdoptions.download_cache) + cmd_opts.add_option(cmdoptions.download_cache.make()) cmd_opts.add_option( '--src', '--source', '--source-dir', '--source-directory', @@ -99,7 +99,7 @@ def __init__(self, *args, **kw): action='store_true', help='Ignore the installed packages (reinstalling instead).') - cmd_opts.add_option(cmdoptions.no_deps) + cmd_opts.add_option(cmdoptions.no_deps.make()) cmd_opts.add_option( '--no-install', @@ -114,8 +114,8 @@ def __init__(self, *args, **kw): help="Don't download any packages, just install the ones already downloaded " "(completes an install run with --no-install).") - cmd_opts.add_option(cmdoptions.install_options) - cmd_opts.add_option(cmdoptions.global_options) + cmd_opts.add_option(cmdoptions.install_options.make()) + cmd_opts.add_option(cmdoptions.global_options.make()) cmd_opts.add_option( '--user', @@ -136,7 +136,7 @@ def __init__(self, *args, **kw): default=None, help="Install everything relative to this alternate root directory.") - cmd_opts.add_option(cmdoptions.use_wheel) + cmd_opts.add_option(cmdoptions.use_wheel.make()) cmd_opts.add_option( '--pre', @@ -144,7 +144,7 @@ def __init__(self, *args, **kw): default=False, help="Include pre-release and development versions. By default, pip only finds stable versions.") - cmd_opts.add_option(cmdoptions.no_clean) + cmd_opts.add_option(cmdoptions.no_clean.make()) index_opts = cmdoptions.make_option_group(cmdoptions.index_group, self.parser) diff --git a/pip/commands/wheel.py b/pip/commands/wheel.py index a29cc2d2059..d90238b0bc5 100644 --- a/pip/commands/wheel.py +++ b/pip/commands/wheel.py @@ -48,17 +48,17 @@ def __init__(self, *args, **kw): metavar='dir', default=DEFAULT_WHEEL_DIR, help="Build wheels into , where the default is '/wheelhouse'.") - cmd_opts.add_option(cmdoptions.use_wheel) + cmd_opts.add_option(cmdoptions.use_wheel.make()) cmd_opts.add_option( '--build-option', dest='build_options', metavar='options', action='append', help="Extra arguments to be supplied to 'setup.py bdist_wheel'.") - cmd_opts.add_option(cmdoptions.requirements) - cmd_opts.add_option(cmdoptions.download_cache) - cmd_opts.add_option(cmdoptions.no_deps) - cmd_opts.add_option(cmdoptions.build_dir) + cmd_opts.add_option(cmdoptions.requirements.make()) + cmd_opts.add_option(cmdoptions.download_cache.make()) + cmd_opts.add_option(cmdoptions.no_deps.make()) + cmd_opts.add_option(cmdoptions.build_dir.make()) cmd_opts.add_option( '--global-option', @@ -74,7 +74,7 @@ def __init__(self, *args, **kw): default=False, help="Include pre-release and development versions. By default, pip only finds stable versions.") - cmd_opts.add_option(cmdoptions.no_clean) + cmd_opts.add_option(cmdoptions.no_clean.make()) index_opts = cmdoptions.make_option_group(cmdoptions.index_group, self.parser) diff --git a/tests/functional/test_help.py b/tests/functional/test_help.py index 3d32db706c1..65c67954998 100644 --- a/tests/functional/test_help.py +++ b/tests/functional/test_help.py @@ -1,7 +1,6 @@ import pytest from pip.exceptions import CommandError -from pip import create_main_parser from pip.basecommand import ERROR, SUCCESS from pip.commands.help import HelpCommand from pip.commands import commands @@ -14,7 +13,7 @@ def test_run_method_should_return_sucess_when_finds_command_name(): """ options_mock = Mock() args = ('freeze',) - help_cmd = HelpCommand(create_main_parser()) + help_cmd = HelpCommand() status = help_cmd.run(options_mock, args) assert status == SUCCESS @@ -25,7 +24,7 @@ def test_run_method_should_return_sucess_when_command_name_not_specified(): """ options_mock = Mock() args = () - help_cmd = HelpCommand(create_main_parser()) + help_cmd = HelpCommand() status = help_cmd.run(options_mock, args) assert status == SUCCESS @@ -36,7 +35,7 @@ def test_run_method_should_raise_command_error_when_command_does_not_exist(): """ options_mock = Mock() args = ('mycommand',) - help_cmd = HelpCommand(create_main_parser()) + help_cmd = HelpCommand() with pytest.raises(CommandError): help_cmd.run(options_mock, args) diff --git a/tests/functional/test_search.py b/tests/functional/test_search.py index 6e6e5e7bcc8..da19487fa82 100644 --- a/tests/functional/test_search.py +++ b/tests/functional/test_search.py @@ -5,7 +5,6 @@ SearchCommand) from pip.status_codes import NO_MATCHES_FOUND, SUCCESS from pip.backwardcompat import xmlrpclib, b -from pip import create_main_parser from mock import Mock from tests.lib import pyversion @@ -87,7 +86,7 @@ def test_run_method_should_return_sucess_when_find_packages(): """ options_mock = Mock() options_mock.index = 'http://pypi.python.org/pypi' - search_cmd = SearchCommand(create_main_parser()) + search_cmd = SearchCommand() status = search_cmd.run(options_mock, ('pip',)) assert status == SUCCESS @@ -98,7 +97,7 @@ def test_run_method_should_return_no_matches_found_when_does_not_find_packages() """ options_mock = Mock() options_mock.index = 'https://pypi.python.org/pypi' - search_cmd = SearchCommand(create_main_parser()) + search_cmd = SearchCommand() status = search_cmd.run(options_mock, ('non-existant-package',)) assert status == NO_MATCHES_FOUND, status From eed12b54c10d0bebbae242b7eb0c9db0f915a624 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Mon, 16 Sep 2013 22:21:21 -0700 Subject: [PATCH 5/6] command parsing tests --- tests/unit/test_basecommand.py | 99 ++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 tests/unit/test_basecommand.py diff --git a/tests/unit/test_basecommand.py b/tests/unit/test_basecommand.py new file mode 100644 index 00000000000..285454cb751 --- /dev/null +++ b/tests/unit/test_basecommand.py @@ -0,0 +1,99 @@ +import os +import copy +from pip import main +from pip import cmdoptions +from pip.basecommand import Command +from pip.commands import InstallCommand +from pip.commands import commands + +class FakeCommand(Command): + name = 'fake' + summary = name + def main(self, args): + return self.parse_args(args) + +class TestCommandParsing(object): + + def setup(self): + self.environ_before = os.environ.copy() + commands[FakeCommand.name] = FakeCommand + + def teardown(self): + os.environ = self.environ_before + commands.pop(FakeCommand.name) + + def different_than_default(self, option): + """ + Generate something different than the option default + Returns a tuple containing: + - the value to test against getattr(options, option.dest) + - the args to achieve it , e.g ['--log', '/path'] + """ + NO_DEFAULT = (option.default == ('NO', 'DEFAULT')) + long_opt = option._long_opts[0] + + if option.type == 'choice': + dest = option.default[:] + dest.append(option.choices[0]) + if dest == option.default: + assert False, "The test is not altering the default" + args = [] + for d in dest: + args.extend([long_opt, d]) + elif option.type in ['str', 'string']: + if NO_DEFAULT: + dest = 'CHANGED' + else: + dest = option.default + 'CHANGED' + args = [long_opt, dest] + elif option.action == 'count': + dest = option.default + 1 + args = [long_opt] + elif option.type == 'float': + dest = option.default + 100 + args = [long_opt, str(dest)] + elif option.action == 'store_true': + dest = not option.default + args = [long_opt] + elif option.default == ('NO', 'DEFAULT'): + dest = 'CHANGED' + args = [long_opt, dest] + + return dest, args + + def test_general_options_dont_have_duplicated_defaults(self): + """ + Confirm that 'append' options don't end up with duplicated defaults. + """ + # It's possible for our general options to be used twice during command + # initialization, e.g. `--exists-action` option will be used twice in + # this case: "pip --exists-action s install somepkg". And if they carry + # state (like append options with [] defaults) then this can be + # trouble. We need to confirm this is not happening. + options, args = main(['--exists-action', 's', 'fake']) + assert options.exists_action == ['s'] # not ['s','s'] + + def test_cli_override_general_options(self): + """ + Test overriding default values for general options using the cli + (not referring to config or environment overrides) + """ + # the reason to specifically test general options is due to the + # extra parsing they receive, and the bugs we've had + for option_maker in cmdoptions.general_group['options']: + option = option_maker.make() + if option.dest in ['help', 'version']: + continue + expected_dest, opt_args = self.different_than_default(option) + + # we're going to check with the args before and after the subcommand + # our parser allows general options to live after the subcommand + for args in (opt_args + ['fake'], ['fake'] + opt_args): + options, args_ = main(args) + msg = "%s != %s with args %s" %( + getattr(options, option.dest), + expected_dest, + args + ) + assert getattr(options, option.dest) == expected_dest, msg + From e272d9001ff39ae7042ec66f3b686d1d88ae4fe7 Mon Sep 17 00:00:00 2001 From: Marcus Smith Date: Mon, 16 Sep 2013 22:45:51 -0700 Subject: [PATCH 6/6] replace `make_option_maker` with `OptionMaker` class --- pip/cmdoptions.py | 87 ++++++++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 43 deletions(-) diff --git a/pip/cmdoptions.py b/pip/cmdoptions.py index 8f731a89a47..1cc4811a18f 100644 --- a/pip/cmdoptions.py +++ b/pip/cmdoptions.py @@ -23,27 +23,28 @@ def make_option_group(group, parser): option_group.add_option(option.make()) return option_group -def make_option_maker(*args, **kwargs): - """Return an object with 'make()' method for creating the option later""" - class OptionMaker(): - @staticmethod - def make(): - args_copy = copy.deepcopy(args) - kwargs_copy = copy.deepcopy(kwargs) - return Option(*args_copy, **kwargs_copy) - return OptionMaker +class OptionMaker(object): + """Class that stores the args/kwargs that would be used to make an Option, + for making them later, and uses deepcopy's to reset state.""" + def __init__(self, *args, **kwargs): + self.args = args + self.kwargs = kwargs + def make(self): + args_copy = copy.deepcopy(self.args) + kwargs_copy = copy.deepcopy(self.kwargs) + return Option(*args_copy, **kwargs_copy) ########### # options # ########### -help_ = make_option_maker( +help_ = OptionMaker( '-h', '--help', dest='help', action='help', help='Show help.') -require_virtualenv = make_option_maker( +require_virtualenv = OptionMaker( # Run only if inside a virtualenv, bail if not. '--require-virtualenv', '--require-venv', dest='require_venv', @@ -51,33 +52,33 @@ def make(): default=False, help=SUPPRESS_HELP) -verbose = make_option_maker( +verbose = OptionMaker( '-v', '--verbose', dest='verbose', action='count', default=0, help='Give more output. Option is additive, and can be used up to 3 times.') -version = make_option_maker( +version = OptionMaker( '-V', '--version', dest='version', action='store_true', help='Show version and exit.') -quiet = make_option_maker( +quiet = OptionMaker( '-q', '--quiet', dest='quiet', action='count', default=0, help='Give less output.') -log = make_option_maker( +log = OptionMaker( '--log', dest='log', metavar='file', help='Log file where a complete (maximum verbosity) record will be kept.') -log_explicit_levels = make_option_maker( +log_explicit_levels = OptionMaker( # Writes the log levels explicitely to the log' '--log-explicit-levels', dest='log_explicit_levels', @@ -85,7 +86,7 @@ def make(): default=False, help=SUPPRESS_HELP) -log_file = make_option_maker( +log_file = OptionMaker( # The default log file '--local-log', '--log-file', dest='log_file', @@ -93,7 +94,7 @@ def make(): default=default_log_file, help=SUPPRESS_HELP) -no_input = make_option_maker( +no_input = OptionMaker( # Don't ask for input '--no-input', dest='no_input', @@ -101,14 +102,14 @@ def make(): default=False, help=SUPPRESS_HELP) -proxy = make_option_maker( +proxy = OptionMaker( '--proxy', dest='proxy', type='str', default='', help="Specify a proxy in the form [user:passwd@]proxy.server:port.") -timeout = make_option_maker( +timeout = OptionMaker( '--timeout', '--default-timeout', metavar='sec', dest='timeout', @@ -116,7 +117,7 @@ def make(): default=15, help='Set the socket timeout (default %default seconds).') -default_vcs = make_option_maker( +default_vcs = OptionMaker( # The default version control system for editables, e.g. 'svn' '--default-vcs', dest='default_vcs', @@ -124,7 +125,7 @@ def make(): default='', help=SUPPRESS_HELP) -skip_requirements_regex = make_option_maker( +skip_requirements_regex = OptionMaker( # A regex to be used to skip requirements '--skip-requirements-regex', dest='skip_requirements_regex', @@ -132,7 +133,7 @@ def make(): default='', help=SUPPRESS_HELP) -exists_action = make_option_maker( +exists_action = OptionMaker( # Option when path already exist '--exists-action', dest='exists_action', @@ -144,7 +145,7 @@ def make(): help="Default action when a path already exists: " "(s)witch, (i)gnore, (w)ipe, (b)ackup.") -cert = make_option_maker( +cert = OptionMaker( '--cert', dest='cert', type='str', @@ -152,14 +153,14 @@ def make(): metavar='path', help = "Path to alternate CA bundle.") -index_url = make_option_maker( +index_url = OptionMaker( '-i', '--index-url', '--pypi-url', dest='index_url', metavar='URL', default='https://pypi.python.org/simple/', help='Base URL of Python Package Index (default %default).') -extra_index_url = make_option_maker( +extra_index_url = OptionMaker( '--extra-index-url', dest='extra_index_urls', metavar='URL', @@ -167,14 +168,14 @@ def make(): default=[], help='Extra URLs of package indexes to use in addition to --index-url.') -no_index = make_option_maker( +no_index = OptionMaker( '--no-index', dest='no_index', action='store_true', default=False, help='Ignore package index (only looking at --find-links URLs instead).') -find_links = make_option_maker( +find_links = OptionMaker( '-f', '--find-links', dest='find_links', action='append', @@ -183,7 +184,7 @@ def make(): help="If a url or path to an html file, then parse for links to archives. If a local path or file:// url that's a directory, then look for archives in the directory listing.") # TODO: Remove after 1.6 -use_mirrors = make_option_maker( +use_mirrors = OptionMaker( '-M', '--use-mirrors', dest='use_mirrors', action='store_true', @@ -191,7 +192,7 @@ def make(): help=SUPPRESS_HELP) # TODO: Remove after 1.6 -mirrors = make_option_maker( +mirrors = OptionMaker( '--mirrors', dest='mirrors', metavar='URL', @@ -199,7 +200,7 @@ def make(): default=[], help=SUPPRESS_HELP) -allow_external = make_option_maker( +allow_external = OptionMaker( "--allow-external", dest="allow_external", action="append", @@ -208,7 +209,7 @@ def make(): help="Allow the installation of externally hosted files", ) -allow_all_external = make_option_maker( +allow_all_external = OptionMaker( "--allow-all-external", dest="allow_all_external", action="store_true", @@ -216,7 +217,7 @@ def make(): help="Allow the installation of all externally hosted files", ) -no_allow_external = make_option_maker( +no_allow_external = OptionMaker( "--no-allow-external", dest="allow_all_external", action="store_false", @@ -224,7 +225,7 @@ def make(): help=SUPPRESS_HELP, ) -allow_unsafe = make_option_maker( +allow_unsafe = OptionMaker( "--allow-insecure", dest="allow_insecure", action="append", @@ -233,7 +234,7 @@ def make(): help="Allow the installation of insecure and unverifiable files", ) -no_allow_unsafe = make_option_maker( +no_allow_unsafe = OptionMaker( "--no-allow-insecure", dest="allow_all_insecure", action="store_false", @@ -241,7 +242,7 @@ def make(): help=SUPPRESS_HELP ) -requirements = make_option_maker( +requirements = OptionMaker( '-r', '--requirement', dest='requirements', action='append', @@ -250,27 +251,27 @@ def make(): help='Install from the given requirements file. ' 'This option can be used multiple times.') -use_wheel = make_option_maker( +use_wheel = OptionMaker( '--use-wheel', dest='use_wheel', action='store_true', help='Find and prefer wheel archives when searching indexes and find-links locations. Default to accepting source archives.') -download_cache = make_option_maker( +download_cache = OptionMaker( '--download-cache', dest='download_cache', metavar='dir', default=None, help='Cache downloaded packages in .') -no_deps = make_option_maker( +no_deps = OptionMaker( '--no-deps', '--no-dependencies', dest='ignore_dependencies', action='store_true', default=False, help="Don't install package dependencies.") -build_dir = make_option_maker( +build_dir = OptionMaker( '-b', '--build', '--build-dir', '--build-directory', dest='build_dir', metavar='dir', @@ -279,7 +280,7 @@ def make(): 'The default in a virtualenv is "/build". ' 'The default for global installs is "/pip_build_".') -install_options = make_option_maker( +install_options = OptionMaker( '--install-option', dest='install_options', action='append', @@ -289,7 +290,7 @@ def make(): "Use multiple --install-option options to pass multiple options to setup.py install. " "If you are using an option with a directory path, be sure to use absolute path.") -global_options = make_option_maker( +global_options = OptionMaker( '--global-option', dest='global_options', action='append', @@ -297,7 +298,7 @@ def make(): help="Extra global options to be supplied to the setup.py " "call before the install command.") -no_clean = make_option_maker( +no_clean = OptionMaker( '--no-clean', action='store_true', default=False,