Normalize management generator acronym casing - #61947
Normalize management generator acronym casing#61947Wei Lim (weikanglim) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 040d809c-7fe6-48a5-a0a5-635dff0547df
|
Azure Pipelines: Successfully started running 2 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This pull request adds deterministic, boundary-aware acronym casing normalization to the Azure .NET management generator so that common acronyms (IP/DB/OS and IPv4/IPv6) are consistently cased after existing naming transformations are applied.
Changes:
- Added a normalization pass to
NameVisitorthat rewritesIp/Db/OsandIpv4/Ipv6(andIpV4/IpV6) to canonical casing only at word boundaries. - Applied the normalization to model names, enum/union type names, and model property names (while keeping enum values/clients/operations/parameters out of scope).
- Expanded and refactored
NameVisitorTeststo cover positive/negative/repeated/boundary cases and to reduce test setup duplication.
Show a summary per file
| File | Description |
|---|---|
| eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/src/Visitors/NameVisitor.cs | Adds acronym normalization rules and applies them to final provider names for models/enums/properties. |
| eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/test/NameVisitorTests.cs | Adds targeted tests for acronym casing normalization and refactors common client setup. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
| "PrivateEndpointConnectionListResult", | ||
| "PrivateLinkResourceListResult" | ||
| }; | ||
| private static readonly (string Source, string Replacement)[] _acronymRenamingRules = |
There was a problem hiding this comment.
Wondering if this should instead go in the base emitter - https://github.com/microsoft/typespec/tree/main/packages/http-client-csharp
These don't seem to be specific to mgmt.
There was a problem hiding this comment.
I was going to suggest we put these in unbranded. This is a dotnet rule not an azure rule.
There was a problem hiding this comment.
That sounds perfectly reasonable. 😃
TBH, part of me wasn't entirely sure where we wanted to enforce "naming conventions," since that can also be an organizational concern.
I was also slightly confused by the presence of linters enforcing the same rules. I had the impression we were thinking of linting public TypeSpec rather than changing its generation rules.
Are we ultimately thinking of removing the linter rules in favor of improvements in the generator? I suspect otherwise the rules could still fire even when the resulting naming is actually valid.
All-in-all, I'm in favor of making the change wherever it makes the most sense, and I personally much prefer having this in base TypeSpec. I mostly tossed this PR out based on my limited understanding of where we wanted this behavior to live. 🗡️
Adds deterministic acronym casing to the Azure management generator for model,
enum/union type, and model-property names.
Ip,Db, andOsbecomeIP,DB, andOSat complete identifierboundaries.
Ipv4/IpV4andIpv6/IpV6becomeIPv4andIPv6.IPV4/IPV6spellings remain unchanged for compatibility.Oslo,Ipsum,Osmosis, anddbzremain unchanged.The normalizer runs after existing management naming transformations, allowing
mechanical C#
clientNameoverrides to be removed after the generator ships.Fixes #61942
Part of #61941