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 test for @include over @internal #371

Closed
wants to merge 6 commits into from
Closed

Conversation

tadelesh
Copy link
Member

@tadelesh tadelesh commented Jul 7, 2023

Cadl Ranch Contribution Checklist:

  • I have written a scenario spec
  • I have meaningful @scenario names. Someone can look at the list of scenarios and understand what I'm covering.
  • I have written a mock API
  • I have used @scenarioDocs for extra scenario description and to tell people how to pass my mock api check.

This test is from openai real use case: Azure/autorest.csharp#3519. The new added test could not be tested by mock server, different languages' SDK should add test for their generated code.
The logic emitter should accomplish:

  1. isInternal will return true for InternaIncludelModel model
  2. isInclude will return true for InternaIncludelModel model
  3. InternaIncludelModel will be included in getAllModels
  4. SDK should generate InternaIncludelModel and make it public to user.

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2023

🦋 Changeset detected

Latest commit: 137e950

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@azure-tools/cadl-ranch-specs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tadelesh tadelesh requested a review from pshao25 July 7, 2023 08:59
@tadelesh tadelesh marked this pull request as ready for review July 7, 2023 09:02
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.

LGTM

Please take care of Timothee's comment.

PS: whether model is output-only or input-output actually affect Java class (guess .NET too), but that's language problem, no need to worry in tsp test.

@doc("This is a model only used by internal operation. Also, it is decorated with @include. It should be generated and exported.")
@global.Azure.ClientGenerator.Core.include
model InternaIncludelModel {
name: string;
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jul 10, 2023

Choose a reason for hiding this comment

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

Should we add a nested model to also test transitivity?

Or, if we are not sure how transitivity works for @include, I am fine to leave as this.

Copy link
Member Author

@tadelesh tadelesh Jul 10, 2023

Choose a reason for hiding this comment

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

I think transitivity should be the work of isInclude (though it seems current TCGC has not implemented it yet). I've created an issue to track.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should figure out this issue now.

@tadelesh tadelesh requested review from qiaozha and msyyc July 10, 2023 02:23
@tadelesh
Copy link
Member Author

@msyyc @qiaozha @pshao25 please review from your language's perspective.

Copy link
Member

@pshao25 pshao25 left a comment

Choose a reason for hiding this comment

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

Checked the user's spec, it has nested model. So let's figure out how to handle it in this PR.

@tadelesh
Copy link
Member Author

Checked the user's spec, it has nested model. So let's figure out how to handle it in this PR.

Added nested model in current test, and a fix for @include transitivity: https://github.com/Azure/typespec-azure/pull/3267.

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.

4 participants