Azure / typespec-azure

About TypeSpec Azure Libraries
https://azure.github.io/typespec-azure/
MIT License
11 stars 36 forks source link

`CustomPage` and `Paged` has the same friendly name #1296

Open ArcturusZhang opened 1 month ago

ArcturusZhang commented 1 month ago

The CustomPage model (here) and Paged model (here) have exactly the same friendlyName, this leads to models with the same name in the SDK and might lead to issues.

Taking a closer look, Page<T> is identical to CustomPage<T> when the second argument is empty. Therefore how about this for a fix: We change Page<T> as an alias of CustomPage<T>:

alias Page<Resource extends Reflection.Model> = CustomPage<Resource>;

which in the functionality's perspective, should not break anything.

ArcturusZhang commented 1 month ago

DotNet generator happens to dodge this bullet because we used a cached based on the name of the model when we convert the information provided by TCGC, and TCGC gives us two different models with the same name, thanks to this name-based cache, dotnet generator could work properly by combine these two models.

We are trying to make a change to use the TCGC types as keys, and uncovers this issue.

msyyc commented 1 month ago

typesepc-python doesn't expose paged models in generated SDK code

archerzz commented 1 month ago

DotNet generator happens to dodge this bullet because we used a cached based on the name of the model when we convert the information provided by TCGC, and TCGC gives us two different models with the same name, thanks to this name-based cache, dotnet generator could work properly by combine these two models.

We are trying to make a change to use the TCGC types as keys, and uncovers this issue.

I guess the CustomPaged<Resource comes first, right? It's a super set of Paged<Resource>. If so, we're luck here. I believe this should be fixed.

ArcturusZhang commented 1 month ago

DotNet generator happens to dodge this bullet because we used a cached based on the name of the model when we convert the information provided by TCGC, and TCGC gives us two different models with the same name, thanks to this name-based cache, dotnet generator could work properly by combine these two models. We are trying to make a change to use the TCGC types as keys, and uncovers this issue.

I guess the CustomPaged<Resource comes first, right? It's a super set of Paged<Resource>. If so, we're luck here. I believe this should be fixed.

Actually no - CustomPage and Page do not have relationship with each other at all. Which one comes first depends on which operation comes the first.

ArcturusZhang commented 1 month ago

cc @timotheeguerin @allenjzhang

markcowl commented 1 month ago

In order for the friendly name to be the same, you would need to use items with the same resource name. Are there actual scenarios where a spec would need to do this to describe API, or is this just trying to detect and prevent a potential spec error?

ArcturusZhang commented 4 weeks ago

In order for the friendly name to be the same, you would need to use items with the same resource name. Are there actual scenarios where a spec would need to do this to describe API, or is this just trying to detect and prevent a potential spec error?

We are doing such thing (using both in one tsp) in one of the cadl ranch cases. I am not sure if it is intentional or accidental, but it will be problematic for SDK generators.

Unifying them would also benefit to simply our APIs without breaking any one I believe. What do you think? @markcowl