Continuing my journey on getting that si command working... long options require the equal sign after them while short options do not even work if you add an equal sign after them. This is awfully confusing.

CommentFileSizeAuthor
#13 options.diff9.91 KBmoshe weitzman

Comments

chx’s picture

For example, using --sites-subdir example.com creates a sites subdir called 1. Srsly.

greg.1.anderson’s picture

Title: Options syntax is very confusing and error prone » Drush should check for missing values for options that require values (e.g. --sites-subdir)

#0 is fairly standard in *nix (c.f. git).

#1 could be fixed with better error handling.

moshe weitzman’s picture

Right, this is standard in Unix AFAIK. I agree that it is confusing and masochistic even. But deviating from Unix standard seems pretty unwise.

chx’s picture

Well, this is never a problem with git because the manpage always gives you the option fully. Say, git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>... no equal sign is needed.--author=<pattern>, --committer=<pattern>. Equal sign is needed. It beats you with a cluestick, basically.

greg.1.anderson’s picture

Issue tags: +Release blocker

In a drush command record, we should add:

      'sites-subdir' => array(
         "Name of directory under 'sites' which should be created. Only needed when the subdirectory does not already exist. Defaults to 'default'",
         "required-value' => 'directory_name',
      ),

Then we could make drush output --sites-subdir= instead of --sites-subdir. Then drush could speak softly and carry a big cluestick too.

The only reason this is going to e a tedious rather than trivial change is that mode help options today use the short form, so they're all going to have to be converted into arrays. I will try to carve out time to do this soon, but I might not be successful in the next week.

greg.1.anderson’s picture

Oh, there are some options where the = is optional. For example, --drupal-project-rename is the same as --drupal-project-rename=drupal. The help should be --drupal-project-rename=, but we do not want to complain if the value is not provided.

Perhaps we need:

'value-description' => 'directory_name'

and

'value' => 'optional'

Valid values for the 'value' help option item could be 'required', 'optional', and 'none'. Default could be 'none' if there is no value-description, and 'required' if value-description is specified.

moshe weitzman’s picture

This will be a lot of work to review existing options and code them properly but I really like the idea. These sorts of improvements go a long long way.

jonhattan’s picture

I've think several times in validating accepted values for command options. Here's a proposal to validate options and its values:

$command['options']['some-option'] => array(
  'description' => '...',
  'value' => no/optional/required,
  'default-value' => NULL / value,
  'default-value-callback' => functionname,
  'valid-values' => NULL / array() / callback,
  'valid-values-callback' => functionname,
),

'default-value' is only useful if 'value' is optional. Even in that case it may be NULL / not provided, meaning that the value is to be calculated later (e.g: si --admin-pass is randomly generated or sql-dump --result-file create a date based filename). An alternative is to provide instead a 'default-value-callback' key.

'valid-values' allow to specify an array of valid values (e.g. pm-download profile --variant=full | project |"profile only"). The callback alternative could be useful for example to validate hostnames in the case of site-dir.

greg.1.anderson’s picture

I don't think it would be advisable to change the function signature for drush_get_option at this stage of drush-5. That means that default values would have to be implemented in terms of drush_set_option on the 'default' context. I don't think I'm excited about attaching callbacks to items in a drush context. I therefore suggest that callbacks is a drush-6 feature, at best. I don't know that I'm excited about doing it there, either.

greg.1.anderson’s picture

I suppose we could call the callback functions as soon as the command record is parsed, and stash the result in the default context. That wouldn't be too hard to manage.

moshe weitzman’s picture

I'd like to focus on chx example from #4. We need a way to show the equal when needed and add a [<treeish>] for example. This issue is mostly about better help, IMO

greg.1.anderson’s picture

Yes, let's keep this issue about better help. I'm mildly negative on the callbacks still, but that can be discussed elsewhere.

moshe weitzman’s picture

Status: Active » Needs review
StatusFileSize
new9.91 KB

We actually already have support for example-value. This patch adds 'value' key defaults to 'required' but can also be 'optional'. Optional keys get square braces and required get angle backets. Patch adds support in core.drush.inc and global options.

I think this is an improvement, but it does duplicate a lot of info already found in description and/or in examples section.

Here is some output:

// From `drush topic core-global-options`
-r <path>, --root=<path>                      Drupal root directory to use (default: current directory).                                                      
 -l <http://example.com:8888>,                 URI of the drupal site to use.     
 --uri=<http://example.com:8888>                                                                                                                               
 -v, --verbose                                 Display extra information about the command.                                                                    
 -d, --debug                                   Display even more information, including internal messages.                                                     
 -y, --yes                                     Assume 'yes' as answer to all prompts.                                                                          
 -n, --no                                      Assume 'no' as answer to all prompts.                                                                           
 -s, --simulate                                Simulate all relevant actions (don't actually change the system).                                               
 -p, --pipe                                    Emit a compact representation of the command for scripting.                                                     
 -h, --help                                    This help system.                                                                                               
 --version                                     Show drush version.                                                                                             
 --php=</path/to/file>                         The absolute path to your PHP intepreter, if not 'php' in the path.                                             
 -ia, --interactive                            Force interactive mode for commands run on multiple targets.               
 -q, --quiet                                   Suppress non-error messages.                                                                                    
 -i </path/to/directory>,                      A list of additional directory paths to search for drush commands.                                              
 --include=</path/to/directory>                                                                                                                                
 -c, --config                                  Specify an additional config file to load. See example.drushrc.php.                                             
 -u <name_or_number>, --user=<name_or_number>
greg.1.anderson’s picture

Awesome. I like it.

moshe weitzman’s picture

Category: bug » feature
Status: Needs review » Active
Issue tags: -Release blocker

Committed in 7dca167. I took a quick run through the rest of the commands and expanded the options that I thought would benefit from example values.

I'm still on the fence about whether we do as chx asked and warn when an option requires a value. This is a pretty big change. We might even have to just mandate that options stop using simple name => description pairs and start using name => array() so that we know for sure which ones take values. I'm thinking that this is a drush6 thing. I'm open to discussion about next steps. Leaving this open for now.

jonhattan’s picture

This commit was causing a lot of

Array to string conversion core.drush.inc:885                                                                     [warning]
Array to string conversion core.drush.inc:894                                                                     [warning]

fixed in http://drupalcode.org/project/drush.git/commit/c1e02f6

greg.1.anderson’s picture

Version: » 8.x-6.x-dev
Status: Active » Closed (won't fix)
Issue tags: +Needs migration

This issue was marked closed (won't fix) because Drush has moved to Github.

If this feature is still desired, you may copy it to our Github project. For best results, create a Pull Request that has been updated for the master branch. Post a link here to the PR, and please also change the status of this issue to closed (duplicate).

Please ask support questions on Drupal Answers.