-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix dnx --help
and tool commands to use valid .NET tool examples
#51009
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
Fix dnx --help
and tool commands to use valid .NET tool examples
#51009
Conversation
This PR is targeting |
@copilot I noticed that you were getting stuck testing your changes. When you need to test a change to a command, you need to build the new 'redist' SDK, and then use the 'dogfood' scripts to create an environment in which you can easily run the newly created CLI commands. To build the new redist SDK, you run |
…amples Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Thank you for the guidance on using the dogfood script! I followed your approach:
The dogfood script works perfectly for testing CLI changes. However, I'm encountering a build/caching issue where the help output still shows the old
I've committed the core changes in 7c016c0. The changes are minimal and targeted:
I've also updated the copilot-instructions to document this dogfood testing approach for future CLI changes. |
…nges Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
dnx --help
suggests use of a package that is not a .NET SDK Tooldnx --help
and tool commands to use valid .NET tool examples
src/Cli/dotnet/CommonArguments.cs
Outdated
public static DynamicArgument<PackageIdentityWithRange> RequiredToolPackageIdentityArgument() => | ||
new("packageId") | ||
{ | ||
Description = CliStrings.ToolPackageIdentityArgumentDescription, | ||
CustomParser = (ArgumentResult argumentResult) => ParsePackageIdentityWithVersionSeparator(argumentResult.Tokens[0]?.Value)!.Value, | ||
Arity = ArgumentArity.ExactlyOne, | ||
}; | ||
|
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 instead of creating a new parallel packageId
argument, use a single shared implementation that takes a description as input. Then update call sites to pass the description that is most appropriate. Don't forget that when you change argument/option names or descriptions, you need to update the cli's snapshot test baselines as well.
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.
Refactored to use a single shared implementation with parameterized description as suggested. The changes in b03b828:
-
Replaced duplicate methods with parameterized versions:
RequiredPackageIdentityArgument(string description)
OptionalPackageIdentityArgument(string description)
- Backward-compatible overloads maintained
-
Updated call sites to pass appropriate descriptions:
- Tool commands (install/update/execute/dnx): use
CliStrings.ToolPackageIdentityArgumentDescription
withdotnetsay@2.1.7
- Package commands: continue using generic description with
Newtonsoft.Json@13.0.3
- Tool commands (install/update/execute/dnx): use
-
Updated CLI snapshot baselines using the MSBuild targets - the snapshot tests now pass and show the tool commands correctly use the new examples.
The CLI completion system now properly distinguishes between tool and package contexts while maintaining a clean, shared implementation.
…eview feedback Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
src/Cli/dotnet/CliStrings.resx
Outdated
<data name="ToolPackageIdentityArgumentDescription" xml:space="preserve"> | ||
<value>Package reference in the form of a package identifier like 'dotnetsay' or package identifier and version separated by '@' like 'dotnetsay@2.1.7'.</value> | ||
</data> |
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 I'm going to nitpick once again. Instead of separate Description strings for tool packages and non-tool-packages, make a single format string with placeholders for a package id and version, and then update the callsites to use the templated format string, passing in the existing/appropriate package id and version.
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.
Refactored to use a single format string with placeholders as suggested. Changes in bc0f3a2:
- Single format string: Replaced duplicate resource strings with
PackageIdentityArgumentDescription
using{0}
and{1}
placeholders - Updated method signatures: Changed CommonArguments methods to accept
examplePackage
andexampleVersion
parameters - Context-specific examples:
- Tool commands:
"dotnetsay", "2.1.7"
- Non-tool commands:
"Newtonsoft.Json", "13.0.3"
(default)
- Tool commands:
- Runtime formatting: Uses
string.Format()
to populate placeholders
This eliminates the duplicate resource string while maintaining clean separation between contexts through parameterization.
…ce strings Co-authored-by: baronfel <573979+baronfel@users.noreply.github.com>
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 fixes the issue where dnx --help
, dotnet tool install --help
, and other tool commands display Newtonsoft.Json
as an example package, which is not a valid .NET tool. The changes implement a parameterized approach to package identity argument descriptions, allowing different examples based on context.
- Refactored
PackageIdentityArgumentDescription
to use placeholders{0}
and{1}
for dynamic package name and version examples - Added parameterized overloads to
CommonArguments
methods while maintaining backward compatibility - Updated tool command parsers to use appropriate .NET tool examples (
dotnetsay@2.1.7
)
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/Cli/dotnet/CliStrings.resx | Modified the base resource string to use format placeholders instead of hardcoded examples |
src/Cli/dotnet/xlf/*.xlf | Updated localization files with new format string and marked as needing translation |
src/Cli/dotnet/CommonArguments.cs | Added parameterized methods for package identity arguments with example package/version parameters |
src/Cli/dotnet/Commands/Tool/Install/ToolInstallCommandParser.cs | Updated to use tool-specific examples (dotnetsay@2.1.7) |
src/Cli/dotnet/Commands/Tool/Update/ToolUpdateCommandParser.cs | Updated to use tool-specific examples (dotnetsay@2.1.7) |
test/dotnet.Tests/CompletionTests/snapshots/zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh | Updated completion test baseline with new tool examples |
.github/copilot-instructions.md | Added documentation for dogfood testing approach for CLI changes |
/backport to release/10.0.1xx |
Started backporting to release/10.0.1xx: https://github.com/dotnet/sdk/actions/runs/18080502142 |
Customer Issue
Customers that read the
dnx
help are guided into using a package that isn't a tool - leading to an immediate failure when usingdnx
. We should instead at least provide examples that are invokable viadnx
. Thanks to @mikekistler for reporting.Description
Tiny help text change to suggest different packages in different scenarios.
Testing
Automated snapshot testing.
Risk
Low - only updates strings.
Copilot details
Fixes the issue where
dnx --help
,dotnet tool install --help
, and other tool commands displayNewtonsoft.Json
as an example package, which is not a valid .NET tool.@baronfel's note: I've reviewed this change and am happy with it. I need a team member review for the actual merge.
Changes Made
PackageIdentityArgumentDescription
with placeholders{0}
and{1}
for package name and version, eliminating duplicate resource stringsRequiredPackageIdentityArgument(string examplePackage, string exampleVersion)
- parameterized versionOptionalPackageIdentityArgument(string examplePackage, string exampleVersion)
- parameterized version"dotnetsay", "2.1.7"
parameters"Newtonsoft.Json", "13.0.3"
parameters (default)string.Format()
to populate placeholders dynamically at runtimeCommands Affected
All tool-related commands now show appropriate .NET tool examples:
dnx --help
dotnet tool install --help
dotnet tool exec --help
dotnet tool execute --help
dotnet tool update --help
Non-tool commands continue to use the generic package examples (e.g.,
dotnet package add --help
still showsNewtonsoft.Json@13.0.3
).The changes maintain backward compatibility while providing more helpful and accurate examples for users working with .NET tools. The implementation uses a single source of truth for the format string, making it more maintainable and eliminating duplicate content.
Fixes #51008
Original prompt