-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Disallow duplicate file-level directives #49308
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
Disallow duplicate file-level directives #49308
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 enforces uniqueness among file-level directives by disallowing duplicates, resolving issue #48842.
- Added tests in DotnetProjectConvertTests.cs to validate duplicate directive detection.
- Introduced deduplication logic using a HashSet with a custom NamedDirectiveComparer in VirtualProjectBuildingCommand.cs.
- Updated localization resources (XLF and RESX files) to include error messages for duplicate directives.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Added tests to confirm duplicate file-level directive errors are raised correctly. |
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Implemented deduplication logic by comparing directive type and name. |
src/Cli/dotnet/Commands/xlf/* | Added DuplicateDirective entries to localization files. |
src/Cli/dotnet/Commands/CliCommandStrings.resx | Introduced the DuplicateDirective resource entry. |
seeing similar CI failures here as in #48387, is there some known issue preventing those dotnet watch tests from succeeding across the board? |
Yes, see #49307. |
@MiYanni for a review, thanks |
Failures are in watch tests which are known, containers test which are known, and a completion test which is known. Merging per request |
Resolves #48842.