659272 - GCLI allow the merging of parameters when using short syntax

Status: RESOLVED WONTFIX

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

For example GCLI currently forces:

$ git -a -m 'blah'

and 

$ tar -x -z -v -f name

It's handy to be able to use the more compact:

$ git -am 'blah'

and 

$ tar -xzvf name

Comment 1

[Joe Walker [:jwalker]]

Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.

Comment 2

[Joe Walker [:jwalker]]

Triage. Filter on PEGASUS.

Comment 3

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 4

[Joe Walker [:jwalker]]

From email, recorded here in case it's useful:

My original thought was to make short params automatic so if you had a command "git commit --all" then "git commit -a" wouldn't require extra work. I abandoned this because:
* The code to work out what do use was hard-ish. Not impossible, but "really do I have to go through these lengths to ensure uniqueness" hard.
* Having the 'user-contract' defined by magic seemed like it was going to be a recipe for annoying users when a latter command update broke an unrelated param shortcut.

So my plan was to add a something to make "short: 'a'" work. And I notice that I even added this to the demo git command [1].

So here's what to do:
* The assign process asks parameters if an argument is 'theirs' [2]. For step one we could just make Parameter.isKnownAs [3] also consider the short syntax.
* Step 2 is harder because the 'assign' process takes arguments out of the array when they're dealt with, and when going "-am" neither --all nor --message completely deals with the '-am' argument. Perhaps we should add a temporary arg.handled property which is only used in Requisition._assign or something like that. Perhaps when you've got a plan, run it past me so I can check that we're wouldn't break anything else!

[1]: https://github.com/joewalker/gcli/blob/master/lib/demo/commands/git.js#L74
[2]: https://github.com/joewalker/gcli/blob/master/lib/gcli/cli.js#L1746
[3]: https://github.com/joewalker/gcli/blob/master/lib/gcli/canon.js#L251

Comment 5

[Joe Walker [:jwalker]]

See also bug 659269 "GCLI should allow commands to customize the short parameter names" which is something of a pre-cursor to this bug.

Comment 6

[Joe Walker [:jwalker]]

Bug 659269 is fixed as part of Bug 900114 which should land soon. This was a quick hack that got some of the way, but didn't end up working.

diff --git a/lib/gcli/cli.js b/lib/gcli/cli.js
index d0c658e..82c906f 100644
--- a/lib/gcli/cli.js
+++ b/lib/gcli/cli.js
@@ -1741,6 +1741,7 @@ Requisition.prototype._assign = function(args) {

   this._unassigned = [];
   var outstanding = [];
+  var assignment;

   if (!this.commandAssignment.value) {
     this._addUnassignedArgs(args);
@@ -1762,7 +1763,7 @@ Requisition.prototype._assign = function(args) {
   // Special case: if there is only 1 parameter, and that's of type
   // text, then we put all the params into the first param
   if (this.assignmentCount === 1) {
-    var assignment = this.getAssignment(0);
+    assignment = this.getAssignment(0);
     if (assignment.param.type.name === 'string') {
       var arg = (args.length === 1) ? args[0] : new MergedArgument(args);
       outstanding.push(this.setAssignment(assignment, arg, noArgUp));
@@ -1825,6 +1826,58 @@ Requisition.prototype._assign = function(args) {
     }
   }, this);

+  // Loop over the remaining args looking for combined short params (e.g. -am)
+  var command = this.commandAssignment.value;
+  var i = 0;
+  shortParamLoop:
+  while (i < args.length) {
+    var text = args[i].text;
+    if (/-[0-9A-Za-z]/.test(text)) {
+      // We need to check that all the short params work before we update
+      // anything so we store the affected assignments here
+      var included = [];
+      // Look at each character in turn, the last is special because it doesn't
+      // have to be a boolean
+      for (var c = 1; c < text.length - 1; c++) {
+        assignment = command.getParameterByShortName(text.charAt(c));
+        if (assignment == null || assignment.type.name !== 'boolean' ||
+            assignment.arg == null || assignment.arg.type !== 'BlankArgument') {
+          break shortParamLoop;
+        }
+        included.push(assignment);
+      }
+
+      // The last letter doesn't have to be a boolean parameter
+      assignment = command.getParameterByShortName(text.length - 1);
+      if (assignment == null || assignment.arg.type !== 'BlankArgument') {
+        break shortParamLoop;
+      }
+
+      var arg;
+      if (assignment.type.name === 'boolean') {
+        included.push(assignment);
+      }
+      else {
+        // Handle non-boolean parameters. At this point we're sure it's going
+        // to work so we can just update the assignment directly
+        var valueArg = null;
+        if (i + 1 <= args.length) {
+          valueArg = args.splice(i, 1)[0];
+        }
+        arg = new NamedArgument(arg, valueArg);
+        outstanding.push(this.setAssignment(assignment, arg, noArgUp));
+      }
+
+      // Finally we need to set all the boolean parameters
+      included.forEach(function(assignment) {
+        arg = new TrueNamedArgument(args[i]);
+        outstanding.push(this.setAssignment(assignment, arg, noArgUp));
+      });
+    }
+
+    i++;
+  }
+
   // What's left are positional parameters: assign in order
   unassignedParams.forEach(function(name) {
     var assignment = this.getAssignment(name);

Comment 7

[Joe Walker [:jwalker]]

Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor

Comment 8

[Joe Walker [:jwalker]]

Triage. Filter on Lobster Thermidor.
Nice idea, but no need to keep on file.