Improve installer selection during upgrade #2570
Conversation
| @@ -98,6 +98,9 @@ namespace AppInstaller::CLI | |||
| Argument::ForType(Args::Type::Log), // -o | |||
| Argument::ForType(Args::Type::Override), | |||
| Argument::ForType(Args::Type::InstallLocation), // -l | |||
| // Alias 'a' already used by --all, so use alternative name "ua" here | |||
There was a problem hiding this comment.
Just putting this out there - -a for --all hasn't appeared in any releases yet, so if @denelon thinks that it would be best to keep -a standard to --architecture then it would probably be best to make that change now rather than later.
-* could be an option, so could -+
There was a problem hiding this comment.
@denelon here is what the discussion is.
Issue: we are adding --architecture arg to upgrade command, --architecture args has alias -a in Install command, but -a is used by --all in upgrade command(this has not been officially released yet), do you think we should change --all alias to another one and make -a the alias of --architecture that's consistent for both Install command and Upgrade command?
I'm leaning towards @Trenly 's suggestion. What do you think?
There was a problem hiding this comment.
We shouldn't break any other existing arguments that work. If necessary, we could look at all the arguments against the 2.0 breaking changes Issue and consider some new aliases. I could see "x" for architecture since most of them are x86, x64, etc... but I want to be careful about an argument's alias on one command having a different meaning on a different command.
There was a problem hiding this comment.
Not that it matters here, but another thing to consider is that winget show and winget list don't currently support the --architecture argument.
I think keeping -a for --architecture makes the most sense for now (especially since --ua could easily be mistyped as -ua which would invoke upgrade all with unknown included). As the upgrade command becomes more complex, I'm sure that flags for --include-pinned and --include-explicit will be wanted, so it may pay to pre-think of what some of those aliases may be
There was a problem hiding this comment.
To be clear, -a for --all in upgrade command is not released in any of the releases yet so it's Not a breaking change to remove -a as alias for --all and give -a back to --architecture.
There was a problem hiding this comment.
Had quick sync with Demitrius, we are fine with reverting previous change and give -a back to --architecture to be consistent
There was a problem hiding this comment.
@Trenly we should think up a reasonable alias for --all "-e" for everything lol!
There was a problem hiding this comment.
Either that or start using emoji
There was a problem hiding this comment.
@Trenly we should think up a reasonable alias for --all "-e" for everything lol!
🤕 -e, --exact
These are the ones currently not in use for upgrade - b, c, d, f, g, j, k, n, p, r, t, u, w, x, y, z
I'd propose:
-r : --all # --recurse as Alternate Name
-x : --include-excluded # once winget exclude is added
-y : --include-explicit # to allow for upgrading of packages where RequireExplicitUpgrade with --all
-u : --include-unknown
-p : --include-pinned # once package pinning is implementedThat would allow users to do winget upgrade -ru to upgrade all packages including those with unknown version of winget upgrade -yupr to upgrade everything except excluded packages
| Argument::ForType(Args::Type::HashOverride), | ||
| Argument::ForType(Args::Type::AcceptPackageAgreements), | ||
| Argument::ForType(Args::Type::AcceptSourceAgreements), | ||
| Argument::ForType(Execution::Args::Type::CustomHeader), | ||
| Argument{ "all", 'a', Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag}, | ||
| Argument{ "include-unknown", 'u', Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag}, | ||
| Argument{ "all", Argument::NoAlias, Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag }, |
There was a problem hiding this comment.
| Argument{ "all", Argument::NoAlias, Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag }, | |
| Argument{ "all", 'r', "recurse", Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag }, |
There was a problem hiding this comment.
--all is already very short; do we need an alias at all?
There was a problem hiding this comment.
Because of the ability to chain flag aliases, it makes sense to have one in my opinion, especially considering future features
| Argument{ "all", 'a', Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag}, | ||
| Argument{ "include-unknown", 'u', Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag}, | ||
| Argument{ "all", Argument::NoAlias, Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag }, | ||
| Argument{ "include-unknown", Argument::NoAlias, Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag }, |
There was a problem hiding this comment.
| Argument{ "include-unknown", Argument::NoAlias, Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag }, | |
| Argument{ "include-unknown", 'u', "unknown", Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag }, | |
| // Below are for future planned features | |
| // Argument{ "include-pinned", 'p', "pinned", Args::Type::IncludePinned, Resource::String::IncludePinnedArgumentDescription, ArgumentType::Flag }, | |
| // Argument{ "include-explicit", 'y', "explicit", Args::Type::IncludeExplicit, Resource::String::IncludeExplicitArgumentDescription, ArgumentType::Flag }, |
| Argument::ForType(Args::Type::HashOverride), | ||
| Argument::ForType(Args::Type::AcceptPackageAgreements), | ||
| Argument::ForType(Args::Type::AcceptSourceAgreements), | ||
| Argument::ForType(Execution::Args::Type::CustomHeader), | ||
| Argument{ "all", 'a', Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag}, | ||
| Argument{ "include-unknown", 'u', Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag}, | ||
| Argument{ "all", Argument::NoAlias, Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag }, |
There was a problem hiding this comment.
--all is already very short; do we need an alias at all?
Microsoft Reviewers: Open in CodeFlow