Allow empty file-level directive values #51078
Conversation
|
This PR is targeting |
There was a problem hiding this comment.
Pull Request Overview
This PR allows file-level directive values to be empty in .NET CLI commands. The change addresses issue #51077 by modifying the directive parsing logic to handle empty values instead of treating them as missing values.
Key Changes
- Updated directive parsing logic to distinguish between missing and empty values
- Added comprehensive test coverage for empty directive values across different directive types
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Modified ParseOptionalTwoParts method to return empty string for whitespace-only values instead of null |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Added test method Directives_EmptyValue to verify empty directive values are handled correctly |
|
@RikkiGibson @333fred @MiYanni for reviews, thanks |
|
@RikkiGibson for a review, thanks |
|
|
||
| var secondPart = i < 0 ? [] : context.DirectiveText.AsSpan((i + 1)..).TrimStart(); | ||
| if (i < 0 || secondPart.IsWhiteSpace()) | ||
| if (i < 0) |
There was a problem hiding this comment.
nit: if I reviewed the original implementation of this method today, I would say to rename this to separatorIndex. As this isn't a loop variable, and its scope is quite broad, encountering this as the first line of the "Files changed", feels a bit opaque :)
Description and customer impact
Fixes #51077.
Empty property values specified via the
#:propertydirective were incorrectly disallowed even though the spec says they should be allowed. This is useful in some scenarios like clearing the defaultTargetFrameworkwhen I want to specifyTargetFrameworks(plural) which is how I discovered this.Regression
No, I think this was always broken.
Risk
Low, a simple change, allowing a scenario that was previously an error.