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

Introduce more usages to describe things related with the states of LRO #1887

Merged
merged 25 commits into from
Dec 4, 2024

Conversation

ArcturusZhang
Copy link
Member

@ArcturusZhang ArcturusZhang commented Nov 19, 2024

Remove this workaround since #311 is closed.
Fixes #1752
Fixes #1530

This PR adds three new usage flags: LroInitial, LroPolling, LroFinalEnvelope.
These new usage flags are previously represented by Output, now the Output flag no longer contain the semantics of these usages.
Therefore, if a model's usage is only LroInitial, this means it is only used as the initial response of an LRO.
if a model has both LroInitial and Output, this means it is used as the initial response of an LRO, and it also used as a normal output in other places.
The final result will not have above new usages unless it participates as those roles as well. It will have output usage.

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

azure-sdk commented Nov 19, 2024

All changed packages have been documented.

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

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

  1. Introduce new usage: LroInitial, LroPolling, LroFinalEnvelope.,> 2. usage and access now properly propagate on polling model, final result and final envelop result of lroMetadata.

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@ArcturusZhang ArcturusZhang changed the title remove the arm lro workaround update the usage and access for more things in lroMetadata Nov 20, 2024
@iscai-msft
Copy link
Contributor

@ArcturusZhang are you ok with this being released as part of the november release? in 2 weeks? If so, I would merge into main. If you'd like us to release this before the big release, can you merge into our release branch release/november-2024 and let us know, then we'll do a tcgc release for you. Thanks for your contribution!

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Nov 21, 2024

The changes in this pr looks good to me. Just want to bring up this related issue: #1750 - If TCGC keeps the current usage and access for LRO intial response, we should have a way to let emitter know when to update access and do not generate this model. But this will make TCGC's access less helpful as emitter still need to update it based on usage.

@ArcturusZhang
Copy link
Member Author

I think per discussion this morning, maybe it is better that we could introduce some new usages for these lro related models since they have different context to use.

Is there any issue to track? (my created issue is closed as planned, feel free to reopen it)

i think we could do the usage split in this pr. @ArcturusZhang thoughts?

yeah I will update those in this PR

@ArcturusZhang ArcturusZhang changed the title update the usage and access for more things in lroMetadata Introduce more usages to describe things related with the states of LRO Nov 26, 2024
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

I don't have opinion on this in Java. You can go ahead if no objection from other languages.

I assume the usage is additive. Meaning we cannot know whether a model is "only used in LroInitial"?

@ArcturusZhang
Copy link
Member Author

I don't have opinion on this in Java. You can go ahead if no objection from other languages.

I assume the usage is additive. Meaning we cannot know whether a model is "only used in LroInitial"?

not quite additive.
The new usage (for example LroInitial) is part of the old Output. New Output no longer contains LroInitial.
Therefore if you see one model only has LroInitial, this means this model is only used as the initial response of an lro. The output now means everything other than those have an explicit definitions such as the lro initial.

@weidongxu-microsoft
Copy link
Member

not quite additive. The new usage (for example LroInitial) is part of the old Output. New Output no longer contains LroInitial. Therefore if you see one model only has LroInitial, this means this model is only used as the initial response of an lro. The output now means everything other than those have an explicit definitions such as the lro initial.

Got it. So only the "final result" would still be marked as usage=Output?

nit, maybe we can call it e.g. OutputLroInitial (kind of grouping the "output" family)?

@ArcturusZhang
Copy link
Member Author

not quite additive. The new usage (for example LroInitial) is part of the old Output. New Output no longer contains LroInitial. Therefore if you see one model only has LroInitial, this means this model is only used as the initial response of an lro. The output now means everything other than those have an explicit definitions such as the lro initial.

Got it. So only the "final result" would still be marked as usage=Output?

nit, maybe we can call it e.g. OutputLroInitial (kind of grouping the "output" family)?

I think it is a good idea to emphasize. what do you think on this suggestion? @tadelesh
An issue would be there are already some usage is part of the input usage but not named in this way (for instance jsonmergepatch)

@tadelesh
Copy link
Member

tadelesh commented Nov 29, 2024

not quite additive. The new usage (for example LroInitial) is part of the old Output. New Output no longer contains LroInitial. Therefore if you see one model only has LroInitial, this means this model is only used as the initial response of an lro. The output now means everything other than those have an explicit definitions such as the lro initial.

Got it. So only the "final result" would still be marked as usage=Output?
nit, maybe we can call it e.g. OutputLroInitial (kind of grouping the "output" family)?

I think it is a good idea to emphasize. what do you think on this suggestion? @tadelesh An issue would be there are already some usage is part of the input usage but not named in this way (for instance jsonmergepatch)

i'm thinking of separating the usage into two layers, which means:

  1. for first layer, we label them all as input, output, etc.
  2. for second layer, we separate output to something like:
enum OutputUsage {
  Normal,
  LroInitial,
  Error,
  ...
}

that will be a breaking change, but much extensible and we should make sure each layer orthogonal. idk if there is good data structure could do that, just a quick thought.
loop in @iscai-msft for thoughts.

@tadelesh
Copy link
Member

tadelesh commented Dec 3, 2024

open this issue to track usage refactor: #1941

@ArcturusZhang ArcturusZhang added this pull request to the merge queue Dec 4, 2024
Merged via the queue into Azure:main with commit 41806ca Dec 4, 2024
23 checks passed
@ArcturusZhang ArcturusZhang deleted the remove-lro-workaround branch December 4, 2024 06:15
markcowl pushed a commit to markcowl/typespec-azure that referenced this pull request Dec 5, 2024
…RO (Azure#1887)

Remove this workaround since Azure#311 is closed.
Fixes Azure#1752
Fixes Azure#1530

This PR adds three new usage flags: `LroInitial`, `LroPolling`,
`LroFinalEnvelope`.
These new usage flags are previously represented by `Output`, now the
`Output` flag no longer contain the semantics of these usages.
Therefore, if a model's usage is only `LroInitial`, this means it is
only used as the initial response of an LRO.
if a model has both `LroInitial` and `Output`, this means it is used as
the initial response of an LRO, and it also used as a normal output in
other places.
The final result will not have above new usages unless it participates
as those roles as well. It will have output usage.
iscai-msft pushed a commit that referenced this pull request Dec 6, 2024
…RO (#1887)

Remove this workaround since #311 is closed.
Fixes #1752
Fixes #1530

This PR adds three new usage flags: `LroInitial`, `LroPolling`,
`LroFinalEnvelope`.
These new usage flags are previously represented by `Output`, now the
`Output` flag no longer contain the semantics of these usages.
Therefore, if a model's usage is only `LroInitial`, this means it is
only used as the initial response of an LRO.
if a model has both `LroInitial` and `Output`, this means it is used as
the initial response of an LRO, and it also used as a normal output in
other places.
The final result will not have above new usages unless it participates
as those roles as well. It will have output usage.
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
7 participants