Skip to content

Comments

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

Merged
haolingdong-msft merged 11 commits intoAzure:mainfrom
haolingdong-msft:getallmodels-name
May 30, 2024
Merged

tsp, use TCGC's name for model property schema and parameter name#2787
haolingdong-msft merged 11 commits intoAzure:mainfrom
haolingdong-msft:getallmodels-name

Conversation

@haolingdong-msft
Copy link
Member

@haolingdong-msft haolingdong-msft commented May 27, 2024

Fix #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

* Defines values for ModelOptionalLiteralOptionalLiteral.
*/
public enum ModelOptionalLiteral {
public enum ModelOptionalLiteralOptionalLiteral {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @srnagar, we now leverage TCGC to generate the name for optional literal enum. And their logic is to concat the literal value to the enum's name. e.g. the definitiion is optionalLiteral?: "optionalLiteral", so another OptionalLiteral is append to the name. Please let me know if this is acceptable or not.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented May 28, 2024

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

target: Model | Union | UnionVariant | Enum | EnumMember | ModelProperty | Scalar | Operation | undefined,

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.

// if no projectedName and friendlyName found, return the name of the target (including special handling for template)
if (
target.kind === "Model" &&
target.templateMapper &&
target.templateMapper.args &&
target.templateMapper.args.length > 0
) {
const tspName = getTypeName(target, this.typeNameOptions);
const newName = getNameForTemplate(target);
this.logWarning(`Rename TypeSpec Model '${tspName}' to '${newName}'`);
return newName;
}
if (!target.name && nameHint) {
const newName = nameHint;
this.logWarning(`Rename anonymous TypeSpec ${target.kind} to '${newName}'`);
return newName;
}

@haolingdong-msft haolingdong-msft changed the title tsp, use TCGC's name for model property schema name and union tsp, use TCGC's name for model property schema name May 29, 2024
@haolingdong-msft
Copy link
Member Author

haolingdong-msft commented May 29, 2024

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

target: Model | Union | UnionVariant | Enum | EnumMember | ModelProperty | Scalar | Operation | undefined,

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.

// if no projectedName and friendlyName found, return the name of the target (including special handling for template)
if (
target.kind === "Model" &&
target.templateMapper &&
target.templateMapper.args &&
target.templateMapper.args.length > 0
) {
const tspName = getTypeName(target, this.typeNameOptions);
const newName = getNameForTemplate(target);
this.logWarning(`Rename TypeSpec Model '${tspName}' to '${newName}'`);
return newName;
}
if (!target.name && nameHint) {
const newName = nameHint;
this.logWarning(`Rename anonymous TypeSpec ${target.kind} to '${newName}'`);
return newName;
}

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
Copy link
Member

weidongxu-microsoft commented May 29, 2024

#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
Copy link
Member Author

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
Copy link
Member

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.

@haolingdong-msft haolingdong-msft marked this pull request as ready for review May 29, 2024 06:29
@haolingdong-msft haolingdong-msft changed the title tsp, use TCGC's name for model property schema name tsp, use TCGC's name for model property schema and parameter name May 30, 2024
@haolingdong-msft haolingdong-msft merged commit b804754 into Azure:main May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TSP, TCGC common layer, use TCGC returned name as much as possible

2 participants