-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Consolidate file-level directive separators #49309
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
Conversation
c455949 to
6db8c27
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.
Pull Request Overview
This PR consolidates and unifies the separators used in file-level directives across the codebase, tests, and documentation. The key changes include updating tests to use “@” for sdk/package directives and “=” for property directives, revising error messages in multiple localization files, and refactoring parsing logic in the VirtualProjectBuildingCommand.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Updated separator for package directives in test cases |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Revised test expectations for sdk, package, and property directives |
| src/Cli/dotnet/Commands/xlf/* | Updated error message text and notes for invalid directive names and missing parts |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Refactored ParseOptionalTwoParts to use an explicit separator and added a regex check for disallowed characters |
| src/Cli/dotnet/Commands/CliCommandStrings.resx | Updated resource strings to match the new directive separator requirements |
| documentation/general/dotnet-run-file.md | Adjusted documentation to reflect the updated usage of separators |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:913
- The updated signature that requires an explicit separator improves clarity. It would be helpful to document this change in the method summary, explicitly noting that 'package' directives use '@' and 'property' directives use '='.
private static (string, string?)? ParseOptionalTwoParts(..., char separator)
| : directiveText.IndexOf(' ', StringComparison.Ordinal); | ||
| var firstPart = i < 0 ? directiveText : directiveText[..i]; | ||
| var i = directiveText.IndexOf(separator, StringComparison.Ordinal); | ||
| var firstPart = (i < 0 ? directiveText : directiveText.AsSpan(..i)).TrimEnd(); |
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.
micro-nit to make it clear that directiveText is always converted to span. Take it or not at your discretion.
| var firstPart = (i < 0 ? directiveText : directiveText.AsSpan(..i)).TrimEnd(); | |
| var firstPart = directiveText.AsSpan(..(i < 0 ? ^0 : i)).TrimEnd(); |
| #:sdk Third 3.0=a/b | ||
| #:package P1 1.0/a=b | ||
| #:package P2 2.0/a=b | ||
| #:property Prop1 = One=a/b |
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.
This shows that if user prefers to do #:property Prop = Value, they can do that with an equivalent result to #:property Prop=Value, right?
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.
Right, there is also test Directives_Whitespace which probably shows this more directly.
|
@MiYanni for a review, thanks |
Co-authored-by: Michael Yanni <MiYanni@microsoft.com>
Resolves #48841.