Skip to content
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

Add a property resultSegments in lroMetadata to show the downstream how to get the final result of a lro #2102

Merged
merged 9 commits into from
Jan 28, 2025

Conversation

ArcturusZhang
Copy link
Member

@ArcturusZhang ArcturusZhang commented Jan 21, 2025

Fixes #2072

The resultPath in LroMetadata uses string as its type.
A string type could mean many things, in this case, for a property, it could have three names (at least): its name in typespec, its clientName, its encodedName.
This PR adds a new property in that model to replace this string type property to avoid such confusion to reference the corresponding property, instead use one specific name of the property.

We could have future work items of changing pageable result in similar ways.

@microsoft-github-policy-service microsoft-github-policy-service bot added the lib:tcgc Issues for @azure-tools/typespec-client-generator-core library label Jan 21, 2025
@azure-sdk
Copy link
Collaborator

azure-sdk commented Jan 21, 2025

All changed packages have been documented.

  • @azure-tools/typespec-client-generator-core
Show changes

@azure-tools/typespec-client-generator-core - feature ✏️

Add a resultSegments property to SdkLroServiceFinalResponse and deprecate resultPath property. Add a resultSegments property to SdkMethodResponse.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Jan 21, 2025

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

with .resultPath we were trying to point users to how to get to the actual result (which is shown in the method's result). How does .resultProperties tell us how to get to the result, instead of what the result is. I hear what you're saying though about confusion over whether the property name is the client name / encoded name etc.

@ArcturusZhang
Copy link
Member Author

ArcturusZhang commented Jan 22, 2025

with .resultPath we were trying to point users to how to get to the actual result (which is shown in the method's result). How does .resultProperties tell us how to get to the result, instead of what the result is. I hear what you're saying though about confusion over whether the property name is the client name / encoded name etc.

this resultProperties tells the consumer on exactly which property to use to get to the result.
We make it an array to be defensive in case in the future lro could have multiple layers like paging info
why this resultProperties is better than resultPath because this array contains the exact same reference of those properties the customer need to use. If they want to use client name, they could get it via .name, if they want to use serialized name, they could get it via .serializationOptions.json.name.

To make your statement more precise:

with .resultPath we were trying to point users to how to get to the actual result (which is shown in the method's result)

it will be:
with .resultProperties we were trying to point users to the properties they want to follow on the model envelopeResult to get to the property that holds the actual result (which is shown in the method's result property)

FYI I am also planning to do the same on paging as well.
We are not doing correct thing on paging for this as well - when there are multiple layers, we concatting the "client names" of those properties by .. This has so many assumptions. There is no guarantee that a program language when accessing a member, has to use . (as far as I know Fortran uses %, but there might be others which is mordern enough). Also an emitter may want to use the serialized names instead of client names.
Therefore as for this matter, I think we should do less, and back to what they really are - as I stated above, this information is pointing those properties, then we should just use the reference of those properties.

@iscai-msft
Copy link
Contributor

oh I see, I got the wrong idea from your PR. Yes this makes sense, so it's a resultPath that instead of a string is a path through the properties. We should discuss this tonight in our office hours. Thanks @ArcturusZhang!

@iscai-msft iscai-msft added this pull request to the merge queue Jan 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 27, 2025
@iscai-msft iscai-msft added this pull request to the merge queue Jan 27, 2025
@iscai-msft iscai-msft removed this pull request from the merge queue due to a manual request Jan 27, 2025
@iscai-msft iscai-msft added this pull request to the merge queue Jan 28, 2025
Merged via the queue into Azure:main with commit 4a3a382 Jan 28, 2025
23 checks passed
@ArcturusZhang ArcturusZhang deleted the fix-2072 branch February 5, 2025 04:39
@ArcturusZhang ArcturusZhang changed the title Add a property resultProperties in lroMetadata to show the downstream how to get the final result of a lro Add a property resultSegments in lroMetadata to show the downstream how to get the final result of a lro Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: TCGC, lroMetadata.finalResponse.resultPath is neither clientName nor serializedName
5 participants