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-models-subpath-in-modular-generation #1915

Merged
merged 11 commits into from
Jul 14, 2023

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Jul 6, 2023

This PR is meant to reflect the suggested changes on adding a models subpath export in the modular generation.
In the previous implementation, we generated src/api/models.ts and export all models to both the api layer index.ts, and the classical client layer.
This PR:

  1. moves all the models into src/models/models.ts, and all the operation options into src/models/options.ts.
  2. removes the export for models in api layer index.ts
  3. changes all the places that refer to models and operation options.

fixes #1913

@qiaozha qiaozha marked this pull request as ready for review July 6, 2023 06:24
Copy link
Member Author

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

@xirzec @mpodwysocki @witemple-msft You may mainly focus on the changes in openai_modular smoke test which are in
packages/typespec-test/test/openai_modular/generated/typespec-ts folder of this repo.

@qiaozha qiaozha requested a review from bterlson July 7, 2023 08:54
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

This looks correct to me. @mpodwysocki does this follow the same conventions as notification hub?

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks good to me! The generated examples are what I expected and this seems to be well aligned with what we have done for NotificationHubs.

Copy link
Contributor

@MaryGao MaryGao left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only have one small concern for the inconsistancy import between the api layer and classical layer for models.

#1915 (comment)

@qiaozha
Copy link
Member Author

qiaozha commented Jul 14, 2023

@bterlson I am going to merge this PR now, feel free to let me know if you found anything, we can always change back 😊

@qiaozha qiaozha merged commit c5f6107 into Azure:main Jul 14, 2023
28 checks passed
@qiaozha qiaozha deleted the add-models-subpath-in-modular-generation branch July 14, 2023 02:48
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.

[DPG] Should have models subpath export in Modular.
3 participants