tool install/update: Support package@version syntax#48085
tool install/update: Support package@version syntax#48085edvilme merged 33 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for the package@version syntax by changing the package argument type from a simple string to a tuple (PackageId, Version) throughout the tool commands. Key changes include:
- Adding a custom parser (ParsePackageIdentity) to extract package id and version.
- Updating all tool commands (install, update, uninstall) to use the new tuple-based package argument.
- Introducing conflict detection to ensure that the --version option is not provided alongside an inline version.
Reviewed Changes
Copilot reviewed 27 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/dotnet/CommonArguments.cs | Added custom parser and conflict check for package@version syntax. |
| src/Cli/dotnet/Commands/Tool/Update/* | Updated command commands to use the tuple-based package argument. |
| src/Cli/dotnet/Commands/Tool/Uninstall/* | Updated uninstall commands to use the new argument syntax. |
| src/Cli/dotnet/Commands/Tool/Install/* | Updated install commands and parser to support package@version syntax. |
| src/Cli/dotnet/Commands/Tool/Install/ParseResultExtension.cs | Adjusted version extraction to consider both inline version and --version option. |
Files not reviewed (5)
- src/Cli/dotnet/CommonLocalizableStrings.resx: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf: Language not supported
src/Cli/dotnet/Commands/Tool/Uninstall/ToolUninstallCommandParser.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/Commands/Tool/Uninstall/ToolUninstallCommandParser.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 24 out of 31 changed files in this pull request and generated no comments.
Files not reviewed (7)
- src/Cli/dotnet/CommonLocalizableStrings.resx: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.it.xlf: Language not supported
- src/Cli/dotnet/xlf/CommonLocalizableStrings.ja.xlf: Language not supported
Comments suppressed due to low confidence (2)
src/Cli/dotnet/Commands/Tool/Install/ParseResultExtension.cs:15
- Accessing the Version property directly and calling ToString() may lead to a null reference exception if Version is null. Consider checking if the Version is non-null before calling ToString(), or use a null-conditional operator.
string packageVersion = parseResult.GetValue(ToolInstallCommandParser.PackageIdentityArgument).Version.ToString() ??
src/Cli/dotnet/Commands/Tool/Uninstall/ToolUninstallCommandParser.cs:15
- [nitpick] Other commands now use a PackageIdentity argument to support the package@version syntax. Consider aligning the uninstall command to use PackageIdentity as well (or document the intentional difference) for consistency.
public static readonly CliArgument<string> PackageIdArgument = new("packageId")
|
/azp run dotnet-sdk-public-ci |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
59e336c to
d83e46f
Compare
d83e46f to
1a65884
Compare
There was a problem hiding this comment.
Copilot reviewed 24 out of 31 changed files in this pull request and generated 1 comment.
Files not reviewed (7)
- src/Cli/dotnet/CliStrings.resx: Language not supported
- src/Cli/dotnet/xlf/CliStrings.cs.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.de.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.es.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.fr.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.it.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.ja.xlf: Language not supported
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Commands/Tool/Uninstall/ToolUninstallCommandParser.cs:14
- [nitpick] If the package@version syntax support should also apply to tool uninstallation, consider aligning the uninstall argument with the PackageIdentityArgument pattern used in install and update commands.
public static readonly CliArgument<string> PackageIdArgument = new("packageId")
Forgind
left a comment
There was a problem hiding this comment.
Mostly blocking because of the NRE potential
There was a problem hiding this comment.
Copilot reviewed 22 out of 30 changed files in this pull request and generated no comments.
Files not reviewed (8)
- src/Cli/dotnet/CliStrings.resx: Language not supported
- src/Cli/dotnet/xlf/CliStrings.cs.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.de.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.es.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.fr.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.it.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.ja.xlf: Language not supported
- src/Cli/dotnet/xlf/CliStrings.ko.xlf: Language not supported
Comments suppressed due to low confidence (2)
src/Cli/dotnet/Commands/Tool/Uninstall/ToolUninstallCommandParser.cs:13
- [nitpick] The uninstall command still uses a string-based PackageIdArgument while install/update commands use a PackageIdentityArgument with package@version support. Confirm that this discrepancy is intended or update uninstall commands accordingly.
public static readonly CliArgument<string> PackageIdArgument = new("packageId") {
src/Cli/dotnet/Commands/Tool/Install/ParseResultExtension.cs:14
- Ensure that the conversion from PackageIdentityArgument.Version to string robustly handles null or malformed version values, especially when falling back to the separate version option.
string packageVersion = parseResult.GetValue(ToolInstallCommandParser.PackageIdentityArgument)?.Version?.ToString() ??
Forgind
left a comment
There was a problem hiding this comment.
Much improved; thanks for the changes
Fixes #48155