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

CLI Look and Feel - Option Groups #744

Merged
merged 23 commits into from Dec 14, 2012

Conversation

Projects
None yet
4 participants
@gvalkov
Contributor

gvalkov commented Dec 10, 2012

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)

@gvalkov gvalkov referenced this pull request Dec 10, 2012

Closed

CLI Look and Feel #743

Show outdated Hide outdated pip/commands/freeze.py
'This option can be used multiple times.')
self.parser.add_option(
pypi_opts = optparse.OptionGroup(self.parser, 'Package Index Options')

This comment has been minimized.

@qwcode

qwcode Dec 10, 2012

Contributor

when "pip list" merges, we'd want to refactor this out, since it uses this group too.

@qwcode

qwcode Dec 10, 2012

Contributor

when "pip list" merges, we'd want to refactor this out, since it uses this group too.

Show outdated Hide outdated pip/baseparser.py
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 11, 2012

Contributor

pip <command> -h seems to be broken

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

no such option: -h
Contributor

qwcode commented Dec 11, 2012

pip <command> -h seems to be broken

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

no such option: -h
Show outdated Hide outdated Makefile
@hltbra

This comment has been minimized.

Show comment
Hide comment
@hltbra

hltbra Dec 12, 2012

Member

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.

Member

hltbra commented Dec 12, 2012

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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 12, 2012

Contributor

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

Contributor

qwcode commented Dec 12, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 12, 2012

Contributor

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

Contributor

qwcode commented Dec 12, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 12, 2012

Member

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

Member

pfmoore commented Dec 12, 2012

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

move command options before general options
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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Dec 12, 2012

Contributor

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

Contributor

gvalkov commented Dec 12, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 12, 2012

Member

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?

Member

pfmoore commented Dec 12, 2012

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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Dec 12, 2012

Contributor

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.

Contributor

gvalkov commented Dec 12, 2012

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

This comment has been minimized.

Show comment
Hide comment
@hltbra

hltbra Dec 12, 2012

Member

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

Member

hltbra commented Dec 12, 2012

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

self.option_groups.pop()
self.option_groups.insert(idx, group)
return group

This comment has been minimized.

@qwcode

qwcode Dec 13, 2012

Contributor

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?

@qwcode

qwcode Dec 13, 2012

Contributor

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?

This comment has been minimized.

@qwcode

qwcode Dec 13, 2012

Contributor

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

@qwcode

qwcode Dec 13, 2012

Contributor

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

This comment has been minimized.

@qwcode

qwcode Dec 13, 2012

Contributor

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

@qwcode

qwcode Dec 13, 2012

Contributor

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

This comment has been minimized.

@gvalkov

gvalkov Dec 13, 2012

Contributor

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.

@gvalkov

gvalkov Dec 13, 2012

Contributor

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.

This comment has been minimized.

@qwcode

qwcode Dec 14, 2012

Contributor

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

@qwcode

qwcode Dec 14, 2012

Contributor

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

This comment has been minimized.

@pfmoore

pfmoore Dec 14, 2012

Member

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:

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

@pfmoore

pfmoore Dec 14, 2012

Member

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:

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

This comment has been minimized.

@qwcode

qwcode Dec 14, 2012

Contributor

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

qwcode Dec 14, 2012

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 14, 2012

Contributor

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

Contributor

qwcode commented Dec 14, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@gvalkov

gvalkov Dec 14, 2012

Contributor

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

Contributor

gvalkov commented Dec 14, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 14, 2012

Contributor

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.

Contributor

qwcode commented Dec 14, 2012

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.

Show outdated Hide outdated pip/__init__.py

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

Merge pull request #744 from gvalkov/cli-optgroups
CLI Look and Feel - Option Groups

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

1 check was pending

default The Travis build is in progress
Details
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Dec 14, 2012

Contributor

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

Contributor

qwcode commented Dec 14, 2012

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

@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