Enable long aliases #2107

Trenly · 2022-04-20T00:45:45Z

Have you signed the Contributor License Agreement?
Are you working against an Issue?

This change allows for argument aliases to be more than 1 character

Microsoft Reviewers: Open in CodeFlow

github-actions · 2022-04-20T02:11:37Z

@check-spelling-bot Report

Unrecognized words, please review:

dast

Previously acknowledged words that are now absent

activatable amd Archs dsc FWW Globals hackathon lww mytool Packagedx parametermap Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:Trenly/winget-cli.git repository
on the LongAlias branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1103380971" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

github-actions · 2022-04-20T02:13:42Z

@check-spelling-bot Report

Unrecognized words, please review:

dast

Previously acknowledged words that are now absent

activatable amd Archs dsc FWW Globals hackathon lww mytool Packagedx parametermap Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:Trenly/winget-cli.git repository
on the LongAlias branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1103383413" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

JohnMcPMS · 2022-04-20T18:51:30Z

    // Parse arguments as such:
    //  1. If argument starts with a single -, only the single character alias is considered.
    //      a. If the named argument alias (a) needs a VALUE, it can be provided in these ways:
    //          -a=VALUE
    //          -a VALUE
    //      b. If the argument is a flag, additional characters after are treated as if they start
    //          with a -, repeatedly until the end of the argument is reached.  Fails if non-flags hit.
    //  2. If the argument starts with a double --, only the full name is considered.
    //      a. If the named argument (arg) needs a VALUE, it can be provided in these ways:
    //          --arg=VALUE
    //          --arg VALUE
    //  3. If the argument does not start with any -, it is considered the next positional argument.
    //  4. If the argument is only a double --, all further arguments are only considered as positional.

Supporting multi-character aliases removes, or at least complicates greatly, this feature:

// b. If the argument is a flag, additional characters after are treated as if they start
// with a -, repeatedly until the end of the argument is reached. Fails if non-flags hit.

I can imagine that this might not be a widely used feature, but it would be a breaking change to remove it. Alternatively, we could say that you can chain together any number of characters after a -, but none of our aliases can StartsWith any of the other aliases. That makes the single character aliases take up a lot of the namespace, and single characters are kind of the ultimate in brevity.

What is the goal in having longer aliases? Based on the change I would assume that --verbose-logs is too long and you have to type it in a situation where setting up tab completion is more work than typing it.

The biggest issue with an alias for this is that it's a universal argument, so it would conflict with any other alias if it were common. But I wouldn't be opposed to an arcane alias for it seeing as it has none today. Something like -+ makes some sense, and is not likely that anyone else would want to use it.

Trenly · 2022-04-20T19:09:42Z

Supporting multi-character aliases removes, or at least complicates greatly, this feature:

I did not realize that was a feature. You are correct that I did not consider it here.

What is the goal in having longer aliases?

The goal here is to allow overall extended functionality. I had simply used verbose as an example. There are use cases where a single letter alias doesn’t make sense, or would conflict with an alias of an already existing argument. This would allow for a wider range of aliases to be available.

A secondary effect here is to make it easier to use in general. Looking from the perspective of a new user, and just as an example, I wouldn’t know that --verbose-logs was a thing, but I would expect (based on powershell and other conventions) that -verbose would be a standard behavior.

ghost · 2022-04-29T00:24:50Z

All CLA requirements met.

github-actions · 2022-04-29T00:26:14Z

@check-spelling-bot Report

Unrecognized words, please review:

strlen

Previously acknowledged words that are now absent

activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:Trenly/winget-cli.git repository
on the LongAlias branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1112772753" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

Trenly · 2022-04-29T00:35:15Z

But I wouldn't be opposed to an arcane alias for it seeing as it has none today. Something like -+ makes some sense, and is not likely that anyone else would want to use it.

I've updated the PR so that any aliases longer than 1 character require the -+ syntax

The other thought I had was that perhaps we could still use -- for long aliases, since at that point it would just be a short argument rather than a long alias;

JohnMcPMS · 2022-05-09T22:34:26Z

I've updated the PR so that any aliases longer than 1 character require the -+ syntax

I meant that the alias for the verbose logs option would be the character +, not that a new syntax should be created.

The other thought I had was that perhaps we could still use -- for long aliases, since at that point it would just be a short argument rather than a long alias;

I am not opposed to alternate names for arguments. You don't even have to write new code for that necessarily (although it is probably the better solution).

JohnMcPMS

Sorry about the delay in reviewing. I do think that the easiest way forward is allowing for alternate names for arguments while leaving the alias character stuff alone.

src/AppInstallerCLICore/Argument.h

github-actions · 2022-05-09T23:08:28Z

@check-spelling-bot Report

Unrecognized words, please review:

pid

Previously acknowledged words that are now absent

activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:Trenly/winget-cli.git repository
on the LongAlias branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1121670122" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

Trenly · 2022-05-09T23:11:07Z

I meant that the alias for the verbose logs option would be the character +, not that a new syntax should be created.

Ahhh, I misunderstood. Removed.

I am not opposed to alternate names for arguments. You don't even have to write new code for that necessarily (although it is probably the better solution).

Made this the functionality. Aliases longer than 1 character now are functionally equivalent to alternate names

src/AppInstallerCLICore/Argument.h

src/AppInstallerCLICore/Command.cpp

JohnMcPMS

The test that ensures that there are no commands with arguments that collide should be updated to check against AlternateName as well.

src/AppInstallerCLICore/Argument.cpp

src/AppInstallerCLICore/Argument.h

src/AppInstallerCLITests/Command.cpp

* Use default std::string_view constructor * Update test cases

JohnMcPMS · 2022-06-07T23:51:03Z

/azp run

azure-pipelines · 2022-06-07T23:51:11Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS · 2022-06-08T04:22:04Z

/azp run

azure-pipelines · 2022-06-08T04:22:13Z

Azure Pipelines successfully started running 1 pipeline(s).

Trenly mentioned this pull request Apr 20, 2022

Implementation for Portable install flow #2078

Merged

Trenly marked this pull request as ready for review April 20, 2022 01:26

Trenly requested a review from a team as a code owner April 20, 2022 01:26

Trenly force-pushed the LongAlias branch from 76ab8d5 to 963728c Compare April 29, 2022 00:24

JohnMcPMS requested changes May 9, 2022

View reviewed changes

src/AppInstallerCLICore/Argument.h Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels May 9, 2022

JohnMcPMS requested changes May 11, 2022

View reviewed changes

src/AppInstallerCLICore/Argument.h Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Argument.h Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label May 11, 2022

Trenly force-pushed the LongAlias branch from c8b0e70 to 8cdd3bb Compare May 12, 2022 05:15

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label May 12, 2022

Trenly requested a review from JohnMcPMS May 12, 2022 05:18

JohnMcPMS requested changes May 12, 2022

View reviewed changes

src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Argument.h Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels May 12, 2022

JohnMcPMS reviewed May 18, 2022

View reviewed changes

src/AppInstallerCLITests/Command.cpp Outdated Show resolved Hide resolved

Trenly added 3 commits June 7, 2022 08:45

Add member variable for alternate names

57cfa21

Address PR Comments

41e8070

* Use default std::string_view constructor * Update test cases

Remove confusing alternate name

e1ac676

Trenly force-pushed the LongAlias branch from 798689e to e1ac676 Compare June 7, 2022 13:49

Trenly added 2 commits June 7, 2022 12:55

Fix Collision Check

1f3b483

Revert change to expect.txt

e4d7a1a

Trenly requested a review from JohnMcPMS June 7, 2022 23:43

Fix Test

b31c7a9

JohnMcPMS approved these changes Jun 16, 2022

View reviewed changes

JohnMcPMS merged commit 5bdc55e into microsoft:master Jun 16, 2022

Trenly deleted the LongAlias branch June 16, 2022 17:53

Trenly mentioned this pull request Jun 22, 2022

Add alternate name to Usage String #2268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable long aliases #2107

Enable long aliases #2107

Trenly commented Apr 20, 2022 •

edited by ghost

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading

JohnMcPMS commented Apr 20, 2022

Trenly commented Apr 20, 2022

ghost commented Apr 29, 2022 •

edited by ghost

Loading

github-actions bot commented Apr 29, 2022 •

edited

Loading

Trenly commented Apr 29, 2022

JohnMcPMS commented May 9, 2022

JohnMcPMS left a comment

github-actions bot commented May 9, 2022 •

edited

Loading

Trenly commented May 9, 2022

JohnMcPMS left a comment

JohnMcPMS commented Jun 7, 2022

azure-pipelines bot commented Jun 7, 2022

JohnMcPMS commented Jun 8, 2022

azure-pipelines bot commented Jun 8, 2022

Enable long aliases #2107

Enable long aliases #2107

Conversation

Trenly commented Apr 20, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

github-actions bot commented Apr 20, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

github-actions bot commented Apr 20, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

JohnMcPMS commented Apr 20, 2022

Trenly commented Apr 20, 2022

ghost commented Apr 29, 2022 • edited by ghost Loading

github-actions bot commented Apr 29, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

Trenly commented Apr 29, 2022

JohnMcPMS commented May 9, 2022

JohnMcPMS left a comment

Choose a reason for hiding this comment

github-actions bot commented May 9, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

Trenly commented May 9, 2022

JohnMcPMS left a comment

Choose a reason for hiding this comment

JohnMcPMS commented Jun 7, 2022

azure-pipelines bot commented Jun 7, 2022

JohnMcPMS commented Jun 8, 2022

azure-pipelines bot commented Jun 8, 2022

Trenly commented Apr 20, 2022 •

edited by ghost

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading

ghost commented Apr 29, 2022 •

edited by ghost

Loading

github-actions bot commented Apr 29, 2022 •

edited

Loading

github-actions bot commented May 9, 2022 •

edited

Loading