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

tool install/update: Support package@version syntax #48085

Merged
merged 33 commits into from
Apr 11, 2025

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Apr 1, 2025

Fixes #48155

image

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Apr 1, 2025
@edvilme edvilme removed the untriaged Request triage from a team member label Apr 2, 2025
@edvilme edvilme marked this pull request as ready for review April 2, 2025 23:32
@Copilot Copilot bot review requested due to automatic review settings April 2, 2025 23:32
Copy link
Contributor

@Copilot Copilot AI left a 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

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@edvilme edvilme requested review from dsplaisted and MiYanni April 7, 2025 22:22
@edvilme edvilme changed the title tool-install: Support package@version syntax tool install/update: Support package@version syntax Apr 7, 2025
@edvilme edvilme requested a review from Copilot April 7, 2025 22:43
Copy link
Contributor

@Copilot Copilot AI left a 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")

@edvilme
Copy link
Member Author

edvilme commented Apr 8, 2025

/azp run dotnet-sdk-public-ci

Copy link

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.

@edvilme edvilme requested review from a team as code owners April 8, 2025 18:36
@edvilme edvilme force-pushed the edvilme-tool-at-version branch from 59e336c to d83e46f Compare April 8, 2025 18:37
@edvilme edvilme removed the request for review from a team April 8, 2025 18:37
@edvilme edvilme force-pushed the edvilme-tool-at-version branch from d83e46f to 1a65884 Compare April 8, 2025 22:17
@edvilme edvilme requested a review from Copilot April 8, 2025 23:38
Copy link
Contributor

@Copilot Copilot AI left a 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")

Copy link
Member

@Forgind Forgind left a 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

@edvilme edvilme requested review from Forgind, baronfel and Copilot April 9, 2025 22:25
Copy link
Contributor

@Copilot Copilot AI left a 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() ??

Copy link
Member

@Forgind Forgind left a 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

@edvilme edvilme enabled auto-merge (squash) April 11, 2025 15:57
@edvilme edvilme merged commit 1ee15a6 into dotnet:main Apr 11, 2025
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for tool@version to tool install commands
5 participants