Skip to content
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

Add command aliases #2390

Merged
merged 15 commits into from
Aug 2, 2022
Merged

Add command aliases #2390

merged 15 commits into from
Aug 2, 2022

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Jul 28, 2022

I have tested that this is functional, and that alias collisions do not occur when commands are at different levels. E.g - winget add <params> does not conflict with winget source add <params>.

I need some guidance on how to implement the unit tests for this - mainly just how to modify the EnsureCommandConsistency test. I presume that the command consistency will need to check against command full names, and that command aliases will have to have a method added to get the alias full name, but I'd like some help and guidance before making those changes

Microsoft Reviewers: Open in CodeFlow

@Trenly
Copy link
Contributor Author

Trenly commented Jul 28, 2022

@ryfu-msft / @JohnMcPMS - Do you have any recommendations on how I should modify the EnsureCommandConsistency test?

@JohnMcPMS
Copy link
Member

JohnMcPMS commented Jul 28, 2022

@denelon for this potential change to the user interface. Just want to make sure that he is aware and agreeable to the aliases being added.

I assume that many of these are less about the amount of typing and more about offering more verbs for a more intuitive experience.

There has been a lack of rigor (mostly from me) about completion, with many new features not being added. I'm not going to hold you to it, but now that I've started thinking about it I see it more.

I would think that the tests should be able to steal from the way that the aliases for arguments are verified:

std::unordered_set<std::string> allArgumentNames;
EnsureStringsAreLowercaseAndNoCollisions(command.FullName() + " argument names", args, GetArgumentName, allArgumentNames);
EnsureStringsAreLowercaseAndNoCollisions(command.FullName() + " argument alternate name", args, GetArgumentAlternateName, allArgumentNames);

[Edit]
Let me be more explicit, by adding a way to do a similar thing but op returns a vector<string>, or by changing all of them to use vector<string> and just having the current ones wrap the single string in a vector.

@Trenly
Copy link
Contributor Author

Trenly commented Jul 28, 2022

I would think that the tests should be able to steal from the way that the aliases for arguments are verified

I was thinking this, but I wasn’t sure quite how to implement it given that arg aliases are all strings, and I chose to implement this as a vector

Let me be more explicit, by adding a way to do a similar thing but op returns a vector, or by changing all of them to use vector and just having the current ones wrap the single string in a vector.

I can fairly easily add a method to get all of the aliasFullNames as a vector, and create a method for EnsureVectorStringsAreLowercaseAndNoCollisions . Thank you for the advice!

@denelon
Copy link
Contributor

denelon commented Jul 28, 2022

I've seen plenty of feedback in this area with the list of Issues. I think it will help in terms of muscle memory for users coming from other package managers.

Can we get a table of the original command and the aliases in the description? That would help with consistency when release notes come out.

How are aliases to be discovered?

Maybe:

winget search -?
Windows Package Manager v1.3.2091
Copyright (c) Microsoft Corporation. All rights reserved.

Searches for packages from configured sources.

usage: winget search [[-q] <query>] [<options>]

The following command aliases are available:
  find

The following arguments are available:
  -q,--query                  The query used to search for a package

The following options are available:
  --id                        Filter results by id
  --name                      Filter results by name
  --moniker                   Filter results by moniker
  --tag                       Filter results by tag
  --command                   Filter results by command
  -s,--source                 Find package using the specified source
  -n,--count                  Show no more than specified number of results (between 1 and 1000)
  -e,--exact                  Find package using exact match
  --header                    Optional Windows-Package-Manager REST source HTTP header
  --accept-source-agreements  Accept all source agreements during source operations

More help can be found at: https://aka.ms/winget-command-search

I expect these would only impact the CLI and not the PowerShell cmdlets as those have a discrete set of approved nouns and verbs.

@JohnMcPMS
Copy link
Member

Agreed, no change to PowerShell.

@Trenly
Copy link
Contributor Author

Trenly commented Jul 29, 2022

Agreed with all points

@ghost ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Jul 29, 2022
@Trenly Trenly marked this pull request as ready for review July 29, 2022 17:25
@Trenly Trenly requested a review from a team as a code owner July 29, 2022 17:25
@Trenly
Copy link
Contributor Author

Trenly commented Jul 29, 2022

There has been a lack of rigor (mostly from me) about completion, with many new features not being added. I'm not going to hold you to it, but now that I've started thinking about it I see it more. - @JohnMcPMS

I added command aliases to completion

How are aliases to be discovered? - @denelon

Added to the command help per the proposal

Can we get a table of the original command and the aliases in the description? That would help with consistency when release notes come out. - @denelon

Original Command New Aliases
search find
install add
upgrade update
uninstall remove, rm
source update source refresh
source remove source rm
list ls
show view
settings config

@florelis
Copy link
Member

Pie in the sky idea: What if instead of adding the aliases, we add a feature where users can create their own aliases, like git does?

@denelon
Copy link
Contributor

denelon commented Jul 29, 2022

I could see something like that as a part of the settings.json file. It's an interesting idea. I'd expect we would need to be fairly strict when defining those aliases, but I like it.

@Trenly
Copy link
Contributor Author

Trenly commented Jul 29, 2022

Pie in the sky idea: What if instead of adding the aliases, we add a feature where users can create their own aliases, like git does?

I think that having that option as a user setting would be good; I think that a combination here would be best, where default aliases are specified as they are here, but if a user specified one that conflicted with a pre-defined alias, the user-defined alias would take precedence. But, I think overall it's an interesting idea and could be a good feature in addition to the standard aliases

@vedantmgoyal9
Copy link
Contributor

vedantmgoyal9 commented Jul 29, 2022

Command Alias
install i
list ls
show view
settings config

src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Command.cpp Show resolved Hide resolved
src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Commands/SearchCommand.h Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Command.h Show resolved Hide resolved
src/AppInstallerCLITests/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLITests/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/AppInstallerStrings.cpp Outdated Show resolved Hide resolved
@wmstack
Copy link

wmstack commented Jul 29, 2022

I suggest that #2302 is considered as well. I feel like user scope and machine scope could also be aliases when installing a package (with -g like NPM and -u to force a user scope package).

@wmstack
Copy link

wmstack commented Jul 29, 2022

If you want to find aliases, I think NPM does a great job with their aliases. They make it so that if you make a typo like instal or isntall it would still work. These could be hidden aliases:

npm install [<package-spec> ...]

aliases: add, i, in, ins, inst, insta, instal, isnt, isnta, isntal, isntall

https://docs.npmjs.com/cli/v8/commands/npm-install

That might be considered butchering the aliases, lol. Otherwise we could have this mechanism that says "did you mean ...", however, I think most of the time the author really means it. A lot of those typos could picked up by the program as known combinations. If we want to keep the command space mostly free, then those can be yield an error, so no one relies on them. However, I think they could ideally always point to install.

Another type of alias that I want to mention is adding s to signify listing:

# equivalent to `winget list`
winget packages

@Trenly
Copy link
Contributor Author

Trenly commented Jul 29, 2022

I suggest that #2302 is considered as well. I feel like user scope and machine scope could also be aliases when installing a package (with -g like NPM and -u to force a user scope package).

Those are flags, and not commands. Therefore, they fall outside the scope of a command alias and cannot be considered as part of this PR

src/AppInstallerCLICore/Command.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Command.cpp Show resolved Hide resolved
src/AppInstallerCLITests/Command.cpp Show resolved Hide resolved
src/AppInstallerCLICore/Command.h Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/AppInstallerStrings.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/AppInstallerStrings.cpp Outdated Show resolved Hide resolved
@ghost 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 Aug 1, 2022
@Trenly Trenly requested a review from JohnMcPMS August 1, 2022 22:38
@JohnMcPMS
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Trenly
Copy link
Contributor Author

Trenly commented Aug 1, 2022

I think I missed a type conversion in the test; I don't think std::string_view can be implicitly cast to std::string; If the build fails, I'll push a fix

@JohnMcPMS
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS JohnMcPMS merged commit a5e202c into microsoft:master Aug 2, 2022
@Trenly Trenly deleted the CommandAliases branch August 2, 2022 18:26
@ppvnf ppvnf mentioned this pull request Sep 21, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
6 participants