-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix model factory parameter naming: change eTag to etag for acronym compliance
#53477
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
Conversation
Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
…eter Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
eTag to etag for acronym compliance
...registry/Azure.ResourceManager.DeviceRegistry/src/Generated/ArmDeviceRegistryModelFactory.cs
Outdated
Show resolved
Hide resolved
...tp-client-csharp-mgmt/generator/Azure.Generator.Management/src/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
…st property - Revert StringExtensions.cs changes as requested - Revert ArmDeviceRegistryModelFactory.cs manual changes - Implement ETag parameter name mapping in ModelFactoryVisitor.cs using a dictionary - Add ETag property to Mgmt-TypeSpec test project to verify the fix - Remove StringExtensionsTests.cs as it's no longer needed - Regenerate test project showing etag parameter working correctly Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
...-client-csharp-mgmt/generator/Azure.Generator.Management/src/Visitors/ModelFactoryVisitor.cs
Outdated
Show resolved
Hide resolved
|
@copilot merge from latest main branch, resolve the conflict and regenerate the test code |
- Instead of manually constructing XmlDocParamStatement objects, use method.Update(signature: method.Signature) to refresh documentation - This is the same pattern used in NameVisitor.cs and is cleaner/simpler - All tests pass (60/60) and generated code still correctly shows etag parameter Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
…tory-etag-parameter # Conflicts: # eng/packages/http-client-csharp-mgmt/generator/TestProjects/Local/Mgmt-TypeSpec/tspCodeModel.json
- Merged latest main branch into the PR branch - Resolved merge conflict in tspCodeModel.json - Regenerated test project to ensure compatibility - All generator tests pass (60/60) - Fix verified: etag parameter still correctly generated Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
Done. Merged from latest main branch, resolved the tspCodeModel.json conflict, and regenerated the test code. All tests pass (60/60) and the etag parameter fix is still working correctly. (commits d095981 and 4daefd3) |
eng/packages/http-client-csharp-mgmt/generator/TestProjects/Local/Mgmt-TypeSpec/foo.tsp
Outdated
Show resolved
Hide resolved
- Changed property name in foo.tsp from eTag to etag (lowercase) - Regenerated test project with updated property name - Model factory still correctly generates etag parameter (lowercase) - All generator tests pass (60/60) Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
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 fixes model factory parameter naming to comply with .NET acronym conventions by changing eTag to etag in camelCase parameter names. The solution implements a property-to-parameter name mapping in ModelFactoryVisitor.cs that specifically handles the ETag property case, ensuring the generated parameter name follows the correct convention.
Key Changes:
- Added dictionary-based property-to-parameter name mapping in ModelFactoryVisitor
- Added test case with lowercase
etagproperty in TypeSpec test project - Method signature update automatically refreshes XML documentation
Reviewed Changes
Copilot reviewed 2 out of 7 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ModelFactoryVisitor.cs | Implements property-to-parameter name mapping with dictionary and parameter name update logic |
| foo.tsp | Adds lowercase etag property to test the parameter name generation |
...-client-csharp-mgmt/generator/Azure.Generator.Management/src/Visitors/ModelFactoryVisitor.cs
Show resolved
Hide resolved
- Added check to compare parameter.Name with newName before updating - Combined the property check, dictionary lookup, and name comparison into a single if condition - Avoids unnecessary parameter updates when the name is already correct - All tests pass (60/60) and generated code verified Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
...csharp-mgmt/generator/TestProjects/Local/Mgmt-TypeSpec/src/Generated/Models/FooProperties.cs
Show resolved
Hide resolved
|
/check-enforcer override |
|
@copilot Merge from latest main and regenerate test |
…tory-etag-parameter # Conflicts: # eng/packages/http-client-csharp-mgmt/generator/TestProjects/Local/Mgmt-TypeSpec/tspCodeModel.json
- Merged latest main branch into the PR branch - Resolved merge conflict in tspCodeModel.json - Regenerated test project to ensure compatibility - All generator tests pass (60/60) - Fix verified: etag parameter still correctly generated Co-authored-by: live1206 <5196139+live1206@users.noreply.github.com>
|
/check-enforcer override |
Model Factory Parameter Naming Fix:
eTag→etagProblem
Model factory methods in Azure Management SDKs were generating parameter names that don't follow .NET naming conventions for acronyms. Specifically, the
ETagproperty was being converted toeTaginstead of the correctetagin camelCase parameter names.Solution
Implemented a property-to-parameter name mapping in
ModelFactoryVisitor.csthat specifically handles theETagproperty case:"ETag"property name to"etag"parameter nameChanges Made
etagproperty to Mgmt-TypeSpec test projectFooPropertiesmodeletagparameter is correctly generatedTest Verification
Added
Azure.Core.eTagproperty (lowercase) to the Mgmt-TypeSpec test project. The generated model factory now correctly shows:Files Modified
eng/packages/http-client-csharp-mgmt/generator/Azure.Generator.Management/src/Visitors/ModelFactoryVisitor.cs- Added parameter name mapping logic with optimized update checkeng/packages/http-client-csharp-mgmt/generator/TestProjects/Local/Mgmt-TypeSpec/foo.tsp- Added etag property (lowercase) for testingFixes #53476
Original prompt
Fixes #53476
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.