-
Notifications
You must be signed in to change notification settings - Fork 5k
Add NameVisitor #50869
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?
Add NameVisitor #50869
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
Implements a NameVisitor
that renames model and property names ending with Url
to Uri
, integrates it into the generator pipeline, and adds a unit test for model renaming.
- Introduces
NameVisitor
with logic to transformUrl
suffix toUri
. - Registers
NameVisitor
inManagementClientGenerator
. - Adds a test to verify model name transformation.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/NameVisitor.cs | Implements logic to detect and rename Url suffixes to Uri . |
src/ManagementClientGenerator.cs | Adds NameVisitor to the generator’s visitor pipeline. |
test/NameVisitorTests.cs | Adds a test confirming model name suffix transformation. |
Comments suppressed due to low confidence (3)
eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/src/NameVisitor.cs:33
- [nitpick] The variable name
i
is ambiguous; consider renaming it to something more descriptive likeuriSuffixChar
orreplacementChar
.
const char i = 'i';
eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/test/NameVisitorTests.cs:11
- The namespace
Azure.Generator.Mgmt.Tests
is inconsistent with the project name (Azure.Generator.Management
); consider renaming it toAzure.Generator.Management.Tests
for consistency.
namespace Azure.Generator.Mgmt.Tests
eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/test/NameVisitorTests.cs:35
- Consider adding a test case to verify that model properties ending with
Url
are also renamed toUri
, since the property rename logic iterates overmodel.Properties
.
Assert.That(transformedModel?.Name, Is.EqualTo("TestModelUri"));
{ | ||
if (TryTransformUrlToUri(property.Name, out var newPropertyName)) | ||
{ | ||
// property.Update(name: newPropertyName); |
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.
The property rename logic is commented out. If renaming model properties is intended, uncomment and implement this line; otherwise, remove the loop to avoid confusion.
// property.Update(name: newPropertyName); | |
property.Update(name: newPropertyName); |
Copilot uses AI. Check for mistakes.
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.
pending on microsoft/typespec#7738
Resolves #50823