Revert "Fix RootNamespace handling for dashes and starting digits in … #50502
+16
−138
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…project …" This reverts commit 184f603.
|
This PR is targeting |
There was a problem hiding this comment.
Pull Request Overview
This PR reverts changes that made RootNamespace handling safer for project names with dashes and starting digits. The revert is based on data showing that while 300k out of 5M projects have dashes, only 6k don't have root namespace, and the change poses risks to resource scenarios that outweigh the benefits.
- Removes enhanced RootNamespace transformation logic that handled dashes and digit prefixes
- Removes comprehensive test coverage for the RootNamespace handling scenarios
- Restores simpler RootNamespace logic that only replaces spaces with underscores
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props | Reverts RootNamespace property logic to only handle spaces, removing dash and digit handling |
| test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildALibrary.cs | Removes test methods and helper function for RootNamespace handling scenarios |
Comment on lines
+1053
to
+1071
| string GetPropertyValue(string propertyName) | ||
| { | ||
| Name = "13monkeys", | ||
| TargetFrameworks = targetFramework, | ||
| }; | ||
|
|
||
| var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: targetFramework); | ||
|
|
||
| // Overwrite the default file. CreateTestProject uses the defined project name for the namespace. | ||
| // We need a buildable project to extract the property to verify it | ||
| // since this issue only surfaces in VS when adding a new class through an item template. | ||
| File.WriteAllText(Path.Combine(testAsset.Path, testProject.Name, $"{testProject.Name}.cs"), @" | ||
| using System; | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace _13monkeys | ||
| { | ||
| public class _13monkeysClass | ||
| { | ||
| public static string Name { get { return ""13monkeys""; } } | ||
| public static List<string> List { get { return null; } } | ||
| } | ||
| }"); | ||
| string projectFolder = Path.Combine(testAsset.Path, testProject.Name); | ||
|
|
||
| var buildCommand = new BuildCommand(testAsset, $"{testProject.Name}"); | ||
| buildCommand | ||
| .Execute() | ||
| .Should() | ||
| .Pass(); | ||
|
|
||
| GetPropertyValue("RootNamespace", projectFolder, testProject.TargetFrameworks).Should().Be("_13monkeys"); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("netcoreapp3.1")] | ||
| [InlineData("netcoreapp5.0")] | ||
| public void It_makes_RootNamespace_safe_when_project_name_has_dashes_and_starts_with_digit(string targetFramework) | ||
| { | ||
| var testProject = new TestProject() | ||
| { | ||
| Name = "13-monkeys-project", | ||
| TargetFrameworks = targetFramework, | ||
| }; | ||
|
|
||
| var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: targetFramework); | ||
|
|
||
| // Overwrite the default file. CreateTestProject uses the defined project name for the namespace. | ||
| // We need a buildable project to extract the property to verify it | ||
| // since this issue only surfaces in VS when adding a new class through an item template. | ||
| File.WriteAllText(Path.Combine(testAsset.Path, testProject.Name, $"{testProject.Name}.cs"), @" | ||
| using System; | ||
| using System.Collections.Generic; | ||
| var getValuesCommand = new GetValuesCommand(Log, projectFolder, | ||
| testProject.TargetFrameworks, propertyName, GetValuesCommand.ValueType.Property) | ||
| { | ||
| Configuration = "Debug" | ||
| }; | ||
|
|
||
| namespace _13_monkeys_project | ||
| { | ||
| public class _13_monkeys_projectClass | ||
| { | ||
| public static string Name { get { return ""13-monkeys-project""; } } | ||
| public static List<string> List { get { return null; } } | ||
| } | ||
| }"); | ||
| string projectFolder = Path.Combine(testAsset.Path, testProject.Name); | ||
| getValuesCommand | ||
| .Execute() | ||
| .Should() | ||
| .Pass(); | ||
|
|
||
| var buildCommand = new BuildCommand(testAsset, $"{testProject.Name}"); | ||
| buildCommand | ||
| .Execute() | ||
| .Should() | ||
| .Pass(); | ||
| var values = getValuesCommand.GetValues(); | ||
| values.Count.Should().Be(1); | ||
| return values[0]; | ||
| } | ||
|
|
||
| GetPropertyValue("RootNamespace", projectFolder, testProject.TargetFrameworks).Should().Be("_13_monkeys_project"); | ||
| GetPropertyValue("RootNamespace").Should().Be("Project_Name_With_Spaces"); |
There was a problem hiding this comment.
[nitpick] The GetPropertyValue method is now a local function but duplicates logic that was previously in a private method. Consider whether this local function approach provides sufficient reusability if similar property extraction is needed elsewhere in the test class.
Copilot uses AI. Check for mistakes.
baronfel
approved these changes
Sep 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #49328
Based on data, 300k out of 5M projects have a dash but only 6k don't have root namespace. This potentially breaks resource scenarios as identified by the runtime team. So the scope of people fixed is probably not worth taking the change versus the risk of breaking customers. Those who use template scenarios can fix their bad namespaces manually as they have been this whole time.
CC @jkotas