CLI Look and Feel - Option Groups by gvalkov · Pull Request #744 · pypa/pip

gvalkov · 2012-12-10T01:39:56Z

This pull requests splits all command options into 'General' and 'Command' options. Output of all help commands can be seen here (generated with make show-help-output)

Note that commands still have to add the option group to the parser once they populate it with options (self.parser.add_option_group).

The show-help-output target makes it easy to see the help output of all non-hidden pip commands.

gvalkov · 2012-12-10T01:48:28Z

pip/commands/freeze.py

I'll admit that this is lame ...

what's lame about it?
in the previous pull you weren't using "Command Options", but rather "Install Options", etc..
why the change? I think I prefer "Install Options", "Freeze Options", etc...

The fact that you have to do it by hand in every command. I think I'll keep self.cmd_opts, but change self.cmd_opts.title in every command. Or initialize the optgroup as OptionGroup(parser, '%s Options' % name.capitalize()). I don't feel like importing optparse in every command module.

qwcode · 2012-12-11T04:13:48Z

pip/baseparser.py

I'd rather see the code import the names it needs from optparse, or just use optparse.make_option.

qwcode · 2012-12-11T04:15:44Z

pip <command> -h seems to be broken

$ pip install -h
Usage: pip install [OPTIONS] PACKAGE_NAMES...

no such option: -h

hltbra · 2012-12-12T00:02:09Z

Makefile

Should this Makefile really get merged into develop branch?

I think not.

Would it make any difference if I turned it into a fabfile instead? It's just for convenience after all.

hltbra · 2012-12-12T00:06:46Z

I questioned you about the Makefile, and it reminds me this man page request issue (#415). It would be great if someone work that out.

qwcode · 2012-12-12T04:03:40Z

@gvalkov lgtm, but one more thought. maybe the general options should be last, and not first.
gem does it that way. what was your thinking on that?

qwcode · 2012-12-12T04:37:09Z

@hltbra, @pfmoore, @pnasrat, hey, are you guys thumbs up on these option groups? I am.
the only remaining issue from me would be whether the general options should be first or last. I'm thinking last?

pfmoore · 2012-12-12T08:07:01Z

Looks good to me. General options at the end seems right to me (if it were only -h and -v I'd be OK with at the start, but there are enough general options that the end seems cleaner (look for example at pip freeze where there are more general options than command-specific ones).

Add a new base parser class 'CustomOptionParser' that provides the 'insert_option_group(idx, *, **)' method for inserting an option group at a specific position.

gvalkov · 2012-12-12T13:16:25Z

@hltbra, I've commented on #415. Wouldn't a Makefile/Fabfile/Rakefile be a useful thing to have for such things? Sticking the man page generation in setup.py wouldn't be a problem, but a Makefile would be a more general solution.

pfmoore · 2012-12-12T13:46:35Z

The makefile won't work on Windows. If you're not expecting it to be cross-platform, why not just make it a shell script so it doesn't look like anything other than a convenience for Unix users?

gvalkov · 2012-12-12T18:28:19Z

What else can you mistake it for? Like you said, this is just a non-portable convenience for Unix users. I see nothing wrong with that (well, except the beautiful way in which Makefiles and multi-line bash mix together). Anyway, I don't feel like debating this any further, so I'll just remove it.

hltbra · 2012-12-12T22:00:16Z

LTGM also, @qwcode, @pfmoore, @gvalkov. This pull request is a great enhancement to pip.

qwcode · 2012-12-13T05:30:19Z

pip/baseparser.py

I just saw this showed up in the last commit. I'm not fond of this add/pop/insert routine.
why not just use this in the command classes? self.parser.option_groups.insert(0, grpname)
if you did want to add a helper, why not just add it into the existing ConfigOptionParser class, and why is the logic add/pop/insert, instead of just insert?

if you just want just move on at this point, I'm ok with merging this, and I'll make some tweeks afterwards.

I'm guessing you did the add/pop/insert routine so it would originally be added using the "public" interface, add_option_group . that makes sense, but you still end up breaking the public interface by doing the pop/insert afterwards. to be pure I think, we'd restructure things so we added them in order only using 'add_option_group'.

There isn't much point in reimplementing most of add_option_group when the only 'stateful' thing it does is append to option_groups. I'm also fine with self.parser.option_group.insert(idx, group), but I really think my solution is good enough, so I'll let you sort it out anyway you find appropriate post-merge.

@pfmoore hey, curious what your response to this thread about option groups is, specifically, whether we should worry about only using "add_option_group" (and not inserting directly into parser.option_groups). I can see in the current optparse code that it doesn't matter, but still concerns me.

I've been watching the comments, but to be honest I don't think it's a huge deal. The code is annoyingly obscure, but I can see the point about not duplicating the whole of add_option_group. It seems to me that one of two things should be done:

Add a comment before the pop/insert, clarifying why it's done this way. This is a short-term solution, but good enough for now.

Longer term, probably in a subsequent commit, refactor the code to make insert_option_group the primitive, and implement add_option_group in terms of it (that'd be a one-liner using a hard-coded idx value, presumably).

If @gvalkov feels like doing the refactoring now, that's great, but I wouldn't hold up the commit for it.

I tinkered with this last night, and what I ended up doing is having the subcommands add/insert groups to a simple list property of the command, and then when command.main() runs, it uses optparse.add_option_group to add to the parser from the list in order. no direct index inserts. but this could happen later outside the scope of this pull.

qwcode · 2012-12-14T06:14:19Z

@gvalkov completion seems to be broken since your last pull:

 File "/home/qwcode/p/pypa/pip2/pip/__init__.py", line 63, in autocomplete
  for opt in subcommand.parser.option_list
AttributeError: type object 'SearchCommand' has no attribute 'parser'

subcommand here is a class, when this is expecting an instance
lets fix this in this pull.

so that's 2 problems I've found (this one, and the pip <command> -h failure), but the tests ran clean, right?

so, we need tests that cover these 2 problems, so we can't break this again, and not notice it.

gvalkov · 2012-12-14T20:36:45Z

@qwcode, completion has been fixed in the latest round of commits.

I was wondering if we could do without showing 'General Options' for every command. If pip --log <arg> install --user <arg> works then maybe we don't have to. This information can be conveyed in the command's usage string, e.g:

pip [options] install [options] <package> [<package> ...]

Anyway, just an idea.

qwcode · 2012-12-14T21:11:51Z

I'm getting ready to do a final check on this and merge.

as for your next round of user-facing help changes, I think it's better to hit the mailing list with various plans, and come to a consensus there, and then the pull is just about implementation details, not a mixture of both.

qwcode · 2012-12-14T22:25:32Z

pip/__init__.py

this didn't just fix completion, it changed it's behavior.
e.g for "pip install --" completion, it used to give install options and general options (excluding those marked as SUPPRESS_HELP)
now it shows everything,
"pip --" completion is still the same in that it still excludes the suppressed ones.

how about let's just fix completion and not change it.
if you want to change it, lets include that in your plans for your next pull.

That wasn't intentional. I think I just forgot to remove one of the lines. Incoming fix!

CLI Look and Feel - Option Groups

qwcode · 2012-12-14T23:22:11Z

Merged. @gvalkov , thanks for being so quick in your responses. I'll update the release notes as well.

gvalkov added 13 commits December 10, 2012 02:50

ensure that there is a newline between usage and options

841ae62

add baseparser options to the 'General Options' group

5f69e25

set indent_increment to 1

33c1019

remove temporary fix

dabedd5

add missing import and remove default help option

8def93c

split install options into 'Command Options' and 'Package Index Options'

2c09666

add the Command.gen_opts option group

b34feac

Note that commands still have to add the option group to the parser once they populate it with options (self.parser.add_option_group).

'Command Options' for the freeze command

192f155

'Command Options' for the search command

6d8b55a

'Command Options' for the zip command

8f00079

'Command Options' for the uninstall command

8b1836b

'Command Options' for the show command

416ab39

add makefile + show-help-output target

98e3ef6

The show-help-output target makes it easy to see the help output of all non-hidden pip commands.

gvalkov mentioned this pull request Dec 10, 2012

CLI Look and Feel #743

Closed

gvalkov reviewed Dec 10, 2012
View reviewed changes

set option group title to '$CommandName Options'

7193bf9

qwcode reviewed Dec 11, 2012
View reviewed changes

pip/baseparser.py Outdated

Copy link

Contributor

qwcode Dec 11, 2012

Choose a reason for hiding this comment

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

I'd rather see the code import the names it needs from optparse, or just use optparse.make_option.

gvalkov added 2 commits December 11, 2012 12:17

s/makeopt/optparse.make_option

64f4cae

fix '-h,--help' option for commands

4ec4692

hltbra reviewed Dec 12, 2012
View reviewed changes

move command options before general options

b26cdee

Add a new base parser class 'CustomOptionParser' that provides the 'insert_option_group(idx, *, **)' method for inserting an option group at a specific position.

remove Makefile

0ef6042

qwcode reviewed Dec 13, 2012
View reviewed changes

gvalkov added 4 commits December 14, 2012 14:45

test if 'pip help <cmd>' == 'pip <cmd> --help'

4c26d11

fix command completion

af64773

factor out common test logic

03a8877

test if 'pip <cmd> --<tab>' properly completes

1502e8e

qwcode reviewed Dec 14, 2012
View reviewed changes

exclude unsuppressed parser options from completion

ba87ff2

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

Merge pull request #744 from gvalkov/cli-optgroups

f689f10

CLI Look and Feel - Option Groups

qwcode merged commit f689f10 into pypa:develop Dec 14, 2012

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 Dec 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwcode commented Dec 11, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hltbra commented Dec 12, 2012

Uh oh!

qwcode commented Dec 12, 2012

Uh oh!

qwcode commented Dec 12, 2012

Uh oh!

pfmoore commented Dec 12, 2012

Uh oh!

gvalkov commented Dec 12, 2012

Uh oh!

pfmoore commented Dec 12, 2012

Uh oh!

gvalkov commented Dec 12, 2012

Uh oh!

hltbra commented Dec 12, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwcode commented Dec 14, 2012

Uh oh!

gvalkov commented Dec 14, 2012

Uh oh!

qwcode commented Dec 14, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwcode commented Dec 14, 2012

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants