-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix --project
and --solution
descriptions for dotnet test
MTP to use "test" instead of "run" and extract shared localization strings
#51052
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
base: release/10.0.1xx
Are you sure you want to change the base?
Conversation
Co-authored-by: Youssef1313 <[email protected]>
--solution
of MTP dotnet test
should be worded similarly to --project
--project
and --solution
descriptions for dotnet test
MTP to use "test" instead of "run"
src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Youssef1313 <[email protected]>
<note /> | ||
</trans-unit> | ||
<trans-unit id="TestCmdProjectDescription"> | ||
<source>Defines the path of the project file to test (folder name or full path). If not specified, it defaults to the current directory.</source> |
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.
<source>Defines the path of the project file to test (folder name or full path). If not specified, it defaults to the current directory.</source> | |
<source>Defines the path of the project file to test. Use path to the project file, or path to the directory containing the project file. If not specified, it defaults to the current directory.</source> |
I would formulate it like this, because the current wording forces me to do too much thinking. It says that I should specify the path to the project, but then it talks about folder name or full path. These two are not the opposites in my eyes,
folder name is not the same as relative or full path to a folder.
and full path, is not the same as relative or full path to the project file.
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 please apply the change in wording to the resources, and re-build so we get the updates.
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.
I was following the same wording as used by dotnet run. I prefer to keep both in sync, so I went ahead and updated it for both test and run.
@jjonescz Any concerns about the resource rewording here?
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.
looks good to me
public static readonly Option<string> ProjectOption = new("--project") | ||
{ | ||
Description = CliCommandStrings.CmdProjectDescription, | ||
Description = CliCommandStrings.TestCmdProjectDescription, |
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.
I have a minor nit: the strings for run
and test
are the same except for the 'verb' used - we've been extracting this pattern out to shared strings and providing the 'verb' as a parameter when creating the Option/Argument.
@copilot can you extract the shared loc string and update the test and run usages of the Project and Solution options to use the shared string, passing in the matching verb?
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.
@baronfel Is it fine if the words "run"/"test" are not localized? Or do we want to also have resources with "single" words for "run" and "test" so they are localized?
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.
good point - @copilot make those single words localizeable 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.
Done in commit 99e653e. Created parameterized resource strings CmdProjectDescriptionFormat
and CmdSolutionDescriptionFormat
, and factory methods CommonOptions.ProjectOption(string verb)
and CommonOptions.SolutionOption(string verb)
. Both test and run commands now use these shared methods with their respective verbs ("test" and "run").
Test command:
--project <PROJECT_PATH> Defines the path of the project file to test. Use path to the project file, or
path to the directory containing the project file. If not specified, it
defaults to the current directory.
Run command:
--project <PROJECT_PATH> Defines the path of the project file to run. Use path to the project file, or
path to the directory containing the project file. If not specified, it
defaults to the current directory.
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.
Done in commit d8074b5. Added localized verb resources VerbRun
and VerbTest
in CliCommandStrings.resx. Updated both test and run command parsers to use these localized strings instead of hardcoded "test" and "run".
Changes:
- Created
VerbRun
andVerbTest
resource strings - Updated
MicrosoftTestingPlatformOptions
to useCliCommandStrings.VerbTest
- Updated
RunCommandParser
to useCliCommandStrings.VerbRun
The verbs are now properly localizable, ensuring consistent translations across all languages.
… verb parameter Co-authored-by: baronfel <[email protected]>
--project
and --solution
descriptions for dotnet test
MTP to use "test" instead of "run"--project
and --solution
descriptions for dotnet test
MTP to use "test" instead of "run" and extract shared localization strings
Co-authored-by: baronfel <[email protected]>
<data name="CmdProjectDescription" xml:space="preserve"> | ||
<value>Defines the path of the project file to run (folder name or full path). If not specified, it defaults to the current directory.</value> | ||
<value>Defines the path of the project file to run. Use path to the project file, or path to the directory containing the project file. If not specified, it defaults to the current directory.</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 This resource is unused now? It can be deleted.
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.
Removed in commit cecdc27. Deleted both CmdProjectDescription
and CommandOptionProjectDescription
resources as they are no longer used after switching to the parameterized CmdProjectDescriptionFormat
.
…on resources Co-authored-by: Youssef1313 <[email protected]>
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 You need to also update:
test/dotnet.Tests/CompletionTests/snapshots/zsh/DotnetCliSnapshotTests.VerifyCompletions.verified.zsh
test/dotnet.Tests/CompletionTests/snapshots/pwsh/DotnetCliSnapshotTests.VerifyCompletions.verified.ps1
Specifically, change:
The path to the project file to run (defaults to the current directory if there is only one project).
to
Defines the path of the project file to run. Use path to the project file, or path to the directory containing the project file. If not specified, it defaults to the current directory.
Co-authored-by: Youssef1313 <[email protected]>
Updated in commit 9b2267f. Both completion test snapshot files now use the new description format: Old description:
New description:
Changed in:
|
@baronfel Is this good to merge? |
@Youssef1313 it needs Tactics approval, so you'd need to send the mail |
Customer Issue
Current description for
--solution
(dotnet test):Current description for
--project
(dotnet test):This is confusing to users because both command-line options accept either full path to project/solution or just the directory. Moreover, the description says "run" instead "test".
Current description for
--project
(dotnet run):We would like to align these for correctness and consistency.
Description
Tiny help text change to --project (both run and test commands) and --solution (test command).
PR link: #51052
Testing
Automated tests were updated.
Risk
Low - only updates CLI description strings.
--solution
and--project
options in MTPdotnet test
use "run" instead of "test"CmdProjectDescription
andCmdSolutionDescription
in CliCommandStrings.resx contain "run" instead of "test"CommandOptionProjectDescription
) which are correctChanges Made:
CmdProjectDescriptionFormat
andCmdSolutionDescriptionFormat
in CliCommandStrings.resx that accept a verb parameterProjectOption(string verb)
andSolutionOption(string verb)
in CommonOptions.csVerbRun
andVerbTest
in CliCommandStrings.resxCommonOptions.ProjectOption(CliCommandStrings.VerbTest)
andCommonOptions.SolutionOption(CliCommandStrings.VerbTest)
CommonOptions.ProjectOption(CliCommandStrings.VerbRun)
CmdProjectDescription
andCommandOptionProjectDescription
Verification:
dotnet test --help
(with MTP enabled) shows "Defines the path of the project file to test..." and "Defines the path of the solution file to test..."dotnet run --help
shows "Defines the path of the project file to run..."Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.