-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
tool install/update: Support package@version syntax #48085
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly blocking because of the NRE potential
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much improved; thanks for the changes
Fixes #48155