Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add NameVisitor #50869

wants to merge 1 commit into from

Conversation

live1206
Copy link
Member

Resolves #50823

@Copilot Copilot AI review requested due to automatic review settings June 25, 2025 09:51
@github-actions github-actions bot added CodeGen Issues that relate to code generation Mgmt This issue is related to a management package. labels Jun 25, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 transform Url suffix to Uri.
  • Registers NameVisitor in ManagementClientGenerator.
  • 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 like uriSuffixChar or replacementChar.
            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 to Azure.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 to Uri, since the property rename logic iterates over model.Properties.
            Assert.That(transformedModel?.Name, Is.EqualTo("TestModelUri"));

{
if (TryTransformUrlToUri(property.Name, out var newPropertyName))
{
// property.Update(name: newPropertyName);
Copy link
Preview

Copilot AI Jun 25, 2025

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.

Suggested change
// property.Update(name: newPropertyName);
property.Update(name: newPropertyName);

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CodeGen Issues that relate to code generation Mgmt This issue is related to a management package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Url should be renamed to Uri
1 participant