As mentioned in #671560: Add a --show-progress option to rsync, it should be possible to define $options that affect only particular drush commands. It is tempting to just blast out something that would support this:

$options['command-options']['rsync']['v'] = TRUE;

This would be pretty easy. However, this is yet another example of a situation where contexts might be useful, a-la #628262: Study fabric and build similar on top of backend.inc, so I will allow some time for contemplation and discussion before jumping to conclusions.

Perhaps both are needed -- first the above, and also some way to define a context that are in essence aliases for collections of options. For example:

$options['command-options']['rsync']['context'] = 'certainly-verbose';

$options['contexts']['certainly-verbose'] = array( 'y', 'v' );

That is, drush --context=certainly-verbose is the same as drush -y -v, and the line above says that we want to be "certainly verbose" every time the rsync command runs. Of course this is a contrived example; actual contexts would ideally be shorter than the options they stipulate!

Along the lines of shorter, would it be reasonable for --certainly-verbose to be equivalent to --context=certainly-verbose? The advantages of the former is that it is shorter, and it is also possible to specify multiple contexts easily on the command line. The downside of the former is that it pollutes the global flag namespace, which is only a problem if users unwisely choose names when they define contexts in their drushrc.php files.

CommentFileSizeAuthor
#12 command-default-options-5.patch14.33 KBgreg.1.anderson
#10 command-default-options-4.patch9.03 KBgreg.1.anderson
#9 command-default-options-3.patch9.03 KBgreg.1.anderson
#4 command-default-options-2.patch5.71 KBgreg.1.anderson
#2 command-default-options.patch4.92 KBgreg.1.anderson

Comments

moshe weitzman’s picture

moshe weitzman commented

IMO we should start with the "tempting" enhancement and defer the context bundles until someone really needs them. Its relatively easy to do bundles on your own in drushrc if someone wants that in the meantime.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Active » Needs review
StatusFileSize
new command-default-options.patch4.92 KB

Okay, that's easy. There are just a couple of limitations. You can't have command-specific control of the 'backend' option (which doesn't make sense anyway), and you can't set the global 'user' login option because you've already passed that stage of the bootstrap by the time you get to command dispatching. I do circle back and re-bootstrap the global options so that you can set flags such as -v and -y in a command-specific way. Finally, I use the mechanism introduced for site aliases to insure that command-specific options are merged together should they happen to be defined in multiple configuration files.

greg.1.anderson’s picture

greg.1.anderson commented
Title: drush should support command-specific options + 'contexts' discussion » drush should support command-specific options
greg.1.anderson’s picture

greg.1.anderson commented
StatusFileSize
new command-default-options-2.patch5.71 KB

Oh, and how about a touch of documentation in example.drushrc.php? Attached.

greg.1.anderson’s picture

greg.1.anderson commented

Perhaps I should add that there is one wrinkle; if you define a binary option such as -v, there is no way to override that selection on the command line. Options such as --key=value can quite easily be overridden, though.

moshe weitzman’s picture

moshe weitzman commented

Nice work.

How hard is it to overcome that wrinkle? Its a gotcha, thats going to lead to fist pounding and support requests.

greg.1.anderson’s picture

greg.1.anderson commented

How should it work? Drush supports the standard single-letter option merging, so drush -v -y is the same as drush -vy. If we introduce drush --no-v, could you also say drush --no-vy, and if so, would you then have to say drush --no--show-progress to turn off the "show progress" flag? There are no standards here, but if we go the route of "--no-xxx", then I'd say don't support merging on "no" options, so drush --no-v and drush --no-show-progress are supported.

As an alternate idea, there could be a new global option that would disable command-specific options. It would have to be something short and easy to type, like -x.

Either way, the code is really easy to write -- it's just a question of how it should work.

greg.1.anderson’s picture

greg.1.anderson commented

On reflection, I'm going to vote for "--no-xxx". This would allow you to use "--no-v" to override $options['v'] = TRUE; in a config file. If we did some sort of "off switch" (-x), it wouldn't really be possible to turn off anything except for the command-specific options.

If you concur, I'll roll a patch. (Aside: this patch would mean that --no-no would become a valid option in drush.)

greg.1.anderson’s picture

greg.1.anderson commented
StatusFileSize
new command-default-options-3.patch9.03 KB

Wrinkles smoothed.

I fixed this as described in #8; I think that overall, this works very well. I also fixed a bug in #4; previously, I was setting command-specific options in the 'default' context, so they would be lower-priority than command line options. However, it is also important that command-specific options be higher-priority than all of the config file contexts, so I defined a new 'command' context and placed it just above the config contexts. I also ironed a couple of wrinkles in context.inc by defining the context names in a single place.

There is still the deep philosophical question of whether the 'command' context should be lower priority or higher priority than the 'alias' context. If you say drush myalias specialcommand, and myalias defines --foo=a and there is a command-specific option that sets --foo=b whenever "specialcommand" is used, should foo be "a" or "b"? I decided that the alias should take precedence, but it could go the other way just by reversing the order of 'alias' and 'command' in the context name list.

greg.1.anderson’s picture

greg.1.anderson commented
StatusFileSize
new command-default-options-4.patch9.03 KB

Whoops. There were a couple of typos in #9 that conspired together to make it appear that the patch was working when in fact it was not quite right. Here's a better one.

moshe weitzman’s picture

moshe weitzman commented

Code looks good. Works well. You should commit this after considering the feedback below ...

In the example.drushrc help, lets add no-verbose in addition to no-v so folks know that either form works. It is a subtle point that you have to be consistent between short form and long form (i.e. -v and --verbose) when using no- syntax. We should probably document that.

When specifying a command specific option, is a command alias allowed? Didn't work for me. If not, we need to allow them or document the requirement. Many people will use 'dl' for command name here.

$options['command-options']['rsync'] = array('show-progress' => TRUE);. Should we remove show-progress from rsync and instead define verbose option for rsync? After all, thats what kicked off this effort ... we should have 2 example command specific options; one thats unary and one not.

Is context precedence well described anywhere. If not, perhaps _drush_context_names() doxygen could be expanded? Not really critical for this issue.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs review » Fixed
StatusFileSize
new command-default-options-5.patch14.33 KB

Committed; patch of final changes attached.

  1. It is in fact unfortunate that --no-v and --no-verbose are different. Fixing this would require changing the way drush handles option synonyms (always ask for one variant in the code, have some lookup table to convert from one form to another). For now, I just documented the limitation.
  2. Added support for command aliases.
  3. Removed --show-progress from rsync in favor of just --verbose
  4. Put two example command-specific options in example.drushrc.php
  5. Context precedence was well-defined at the top of the file, but I moved the description down to drush_context_names() and added a forward-reference to the top of the file. Took off the "_" so that we wouldn't have important documentation attached to a private function. Someone might want to know the list of context names, anyway...

Should be fine now, but go ahead and re-open if you'd like any of the above items adjusted.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mightyiam’s picture

mightyiam commented
Component: Code » PM (dl, en, up ...)

Excuse me if I don't get it correctly. I'm just starting out with drush now. Is it possible to use the command-specific options feature to specify in my ~/.drush/drushrc.php that rsync will run with my custom "mode"?
I mean like the "drush rsync --mode=xyz" option.

It isn't clear to me from the docs how to do that, if at all.

greg.1.anderson’s picture

greg.1.anderson commented

$command_specific['rsync']['mode'] = 'xyz';