-
Notifications
You must be signed in to change notification settings - Fork 829
Add project name normalization to match aspire's code generator logic #6818
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: main
Are you sure you want to change the base?
Conversation
…-aspire projects. Add execution tests for the fix.
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 addresses build errors that occur when using the --aspire
flag with project names containing dots or other non-identifier characters. The fix applies Aspire's project name normalization logic to templates to ensure generated code references match the normalized class names.
- Adds project name normalization transforms to template configuration
- Introduces a new test case to verify the fix works with project names containing dots
- Ensures consistency between template-generated code and Aspire's AppHost target behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
AIChatWebExecutionTests.cs | Adds test case for project names with dots to verify normalization fix |
template.json | Implements project name normalization transforms using regex replacement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ates/Microsoft.Extensions.AI.Templates/src/ChatWithCustomData/.template.config/template.json
Outdated
Show resolved
Hide resolved
...ates/Microsoft.Extensions.AI.Templates/src/ChatWithCustomData/.template.config/template.json
Outdated
Show resolved
Hide resolved
...ojectTemplates/Microsoft.Extensions.AI.Templates.IntegrationTests/AIChatWebExecutionTests.cs
Outdated
Show resolved
Hide resolved
...ojectTemplates/Microsoft.Extensions.AI.Templates.IntegrationTests/AIChatWebExecutionTests.cs
Outdated
Show resolved
Hide resolved
"forms": { | ||
"projectNameTransform_Normalize": { | ||
"identifier": "replace", | ||
"pattern": "[^a-zA-Z0-9_]", |
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.
Why is this pattern different from the pattern that the Aspire code generator uses in their _GeneratedClassNameFixupRegex
? If our pattern matches differently from theirs in any scenario, different flavors of this bug will remain.
If this pattern is a simplified version of their pattern that works completely, then the description below should be expanded to explain why our pattern is different from theirs. Imagine their pattern being updated in the future for some reason; we'd need to know how to reapply our fix here to match their updated logic.
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.
Great catch! Using the same regex in one step wasn't working for getting the same transformation but, after checking it again, I was indeed leaving some cases out with the reduction. I now split aspires regex logic in 2 steps instead and it works completely. Thanks!
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.
Terrific. The test should cover each element of logic from the regex by using a name that matches each class of character the regex does a replace on while also verifying things that should not be matched/replaced. So any of these cases you found during re-test can be added into that name used in the unit test.
"type": "derived", | ||
"valueSource": "name", | ||
"valueTransform": "projectNameTransform_Formatting", | ||
"replaces": "ChatWithCustomData_CSharp_Web" |
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 suggest updating the template to use a different (Aspire-specific) token here, similar to how we have a data-ChatWithCustomData-CSharp.Web-
token above, so that it's less likely we'll introduce a false-positive match on this token in the future where this replacer shouldn't apply.
"replaces": "ChatWithCustomData_CSharp_Web" | |
"replaces": "ChatWithCustomData_CSharp_Web_AspireClassName" |
(And then use that token in the template source file as well).
...ates/Microsoft.Extensions.AI.Templates/src/ChatWithCustomData/.template.config/template.json
Outdated
Show resolved
Hide resolved
[Theory] | ||
[InlineData("Ollama")] | ||
[InlineData("OpenAI")] | ||
[InlineData("AzureOpenAI")] | ||
[InlineData("GitHubModels")] | ||
public async Task CreateRestoreAndBuild_ProjectNameVariants(string provider) | ||
{ | ||
string[] projectNames = new[] | ||
{ | ||
"dot.name", | ||
"space name", | ||
"mix.ed-dash_name 123", | ||
".1My.Projec-", | ||
"1Project123", | ||
"project.123" | ||
}; |
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.
You should create a new MemberData method that combines projectNames
with providers.
public static IEnumerable<object[]> GetProjectNameVariants()
{
foreach (string provider in new[] { "Ollama", "OpenAI", "AzureOpenAI", "GitHubModels" })
{
foreach (string projectName in new[]
{
"dot.name",
"space name",
"mix.ed-dash_name 123",
".1My.Projec-",
"1Project123",
"project.123"
})
{
yield return new object[] { provider, projectName };
}
}
}
[Theory]
[MemberData(nameof(GetProjectNameVariants))]
public async Task CreateRestoreAndBuild_ProjectNameVariants(string provider, string projectName)
{
// ...
}
"mix.ed-dash_name 123", | ||
".1My.Projec-", | ||
"1Project123", | ||
"project.123" |
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.
Can you add a more comprehensive theory, e.g.
// leading
.1abc // dot number is replaced with two underscores + number
.abc
#abc // alt character to dot
..abc // double non-word char
##abc
.1.2abc
// mid
abc.1abc
abc.abc
abc#abc
// trailing
// trailing dots in windows are non-standard, so we test with an alt character
abc#
abc##
abc.1
abc.1.2
// single non-word character
#
// no match
abc
_a_b_c
abc1
Abc1abc
Fixes #6811
Apply project name transformation in the templates' generated code using the same normalization logic as in
Aspire.Hosting.AppHost.targets
. This ensures that when the--aspire
flag is used, the generatedAppHost/Program.cs
references normalized class names (e.g.,some_name
instead ofsome.name
), resolving build errors caused by mismatched type or namespace references.Add execution tests to verify the fix for templates with dots or other non-identifier characters in their names.
Microsoft Reviewers: Open in CodeFlow