search

Azure / autorest.java

Extension for AutoRest (https://github.com/Azure/autorest) that generates Java code
MIT License
32 stars 80 forks source link

tsp, use TCGC's name for model property schema and parameter name #2787

Closed haolingdong-msft closed 1 month ago

haolingdong-msft commented 1 month ago

Fix https://github.com/Azure/autorest.java/issues/2731

Code Changes:

  1. use TCGC's name for model property schema name, this would apply to optional literal case.
  2. use TCGC's parameter name
weidongxu-microsoft commented 1 month ago

Can we now remove the Model/Scalar related types from getName, as we should be using TCGC names from Model? https://github.com/Azure/autorest.java/blob/894501332ea54568eb091621f17ffe8895fa57d2/typespec-extension/src/code-model-builder.ts#L2246 We probably still need ModelProperty and Operation as they relates to op.

Also, if above is true, we should be able to remove the "template" related code and "empty name" related code as well. https://github.com/Azure/autorest.java/blob/894501332ea54568eb091621f17ffe8895fa57d2/typespec-extension/src/code-model-builder.ts#L2274-L2291

haolingdong-msft commented 1 month ago

Can we now remove the Model/Scalar related types from getName, as we should be using TCGC names from Model?

https://github.com/Azure/autorest.java/blob/894501332ea54568eb091621f17ffe8895fa57d2/typespec-extension/src/code-model-builder.ts#L2246

We probably still need ModelProperty and Operation as they relates to op. Also, if above is true, we should be able to remove the "template" related code and "empty name" related code as well.

https://github.com/Azure/autorest.java/blob/894501332ea54568eb091621f17ffe8895fa57d2/typespec-extension/src/code-model-builder.ts#L2274-L2291

Thanks for the suggestion. I tried locally, we can remove Scalar and undefined, but probabyly cannot remove Model for now, because there are places that uses Model type:

  1. https://github.com/Azure/autorest.java/blob/main/typespec-extension/src/code-model-builder.ts#L1590
  2. https://github.com/Azure/autorest.java/blob/main/typespec-extension/src/code-model-builder.ts#L1341 (body is Model | ModelProperty)
weidongxu-microsoft commented 1 month ago

1 is just a trace/log, and it is already a Model, you can get TCGC model and use its name (just don't convert to ObjectSchema)

2 is the uncompleted migrate, it should use TCGC model name, if Union is Model (you can call getName if it is ModelProperty; just do a if/else)

haolingdong-msft commented 1 month ago

Yes, I can do if/else, just thinking we can get rid of the whole getName function when integrating with sdkPackage, so I don't have very strong intention of modifying the logics. But I'm fine of doing this in this pr.

weidongxu-microsoft commented 1 month ago

Yes, I can do if/else, just thinking we can get rid of the whole getName function when integrating with sdkPackage, so I don't have very strong intention of modifying the logics. But I'm fine of doing this in this pr.

If you can finish the work next week, I am good with "no very strong intention". If you cannot finish next month, it is good incentive for me to clean that up. The point is this getName could be vastly simplified, if it does not need to work on Model.