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.

Sign up for GitHub

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

Jump to bottom

[Storage][File] Explicitly re-export model types #5532

Merged
merged 1 commit into from Oct 21, 2019
Merged

[Storage][File] Explicitly re-export model types #5532

merged 1 commit into from Oct 21, 2019

Conversation

jeremymeng
Copy link
Contributor

@jeremymeng jeremymeng commented Oct 11, 2019 •
edited

This PR replaces import * as Model with explicitly importing types that we used in public api surface. A geneartedModels.ts is added to import those types aliased with Model suffix to indicate that they are from generated models.

This change brings several benefits, among them

  • api-extractor now works fine to generate the api review markdown file under review folder. It is used to audit the public api changes.
  • we now selectively export types from generated code instead of exporting everything.

@jeremymeng
Copy link
Contributor Author

@bterlson

bterlson
bterlson requested changes Oct 11, 2019
View reviewed changes
Copy link
Member

@bterlson bterlson left a comment

Choose a reason for hiding this comment

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

I think I'd prefer a Model suffix to Models prefix, but otherwise, this is great!

@jeremymeng
Copy link
Contributor Author

I think I'd prefer a Model suffix to Models prefix,

Should we add the suffix to all the types from generated/src/models to make it clear that they are different?

@bterlson
Copy link
Member

I think it's a good idea if you agree!

@jeremymeng jeremymeng marked this pull request as ready for review October 11, 2019 23:55
@jeremymeng jeremymeng changed the title Imports individual model types [Storage][File] Imports individual model types Oct 11, 2019
@jeremymeng
Copy link
Contributor Author

/azp run js - storage-file - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng
Copy link
Contributor Author

need to re-generate the recordings

@XiaoningLiu XiaoningLiu added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 12, 2019
XiaoningLiu
XiaoningLiu approved these changes Oct 12, 2019
View reviewed changes
Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

Looks great!

@jeremymeng jeremymeng changed the title [Storage][File] Imports individual model types [Storage][File] Explicitly re-export model types Oct 15, 2019
@jeremymeng
Copy link
Contributor Author

After adding Model suffix to all model types. I felt it's too much. I am inclining to revert back to only adding it to model types that have conflicting names in convenience layer.

jiacfan
jiacfan reviewed Oct 16, 2019
View reviewed changes
sdk/storage/storage-file/review/storage-file.api.md
@@ -0,0 +1,1261 @@
## API Report File for "@azure/storage-file"

> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/).
Copy link
Member

Choose a reason for hiding this comment

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

Is API extractor invoked in build time? Would better to ensure this tool is embedded properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add it as part of the build like keyvault libraries did. The problem was api-extractor never worked on storage code base before this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logged #5641

jiacfan
jiacfan approved these changes Oct 16, 2019
View reviewed changes
Copy link
Member

@jiacfan jiacfan left a comment

Choose a reason for hiding this comment

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

:shipit:

@jeremymeng jeremymeng changed the base branch from feature/storage to master October 16, 2019 23:16
@jeremymeng jeremymeng force-pushed the imports-individual-model-types branch from 1502a04 to 226a39e Compare October 17, 2019 23:21
@jeremymeng
Copy link
Contributor Author

/azp run js - storage-file - tests

@jeremymeng jeremymeng force-pushed the imports-individual-model-types branch 2 times, most recently from 09e9d85 to 66e2c72 Compare October 19, 2019 03:41
@jeremymeng jeremymeng force-pushed the imports-individual-model-types branch from 66e2c72 to 0fe9acd Compare October 20, 2019 08:00
@ramya-rao-a
Copy link
Contributor

@jeremymeng This is ready to be merged right? I see all relevant approvals and all tests have passed.

@ramya-rao-a
Copy link
Contributor

Related to #5616

@jeremymeng jeremymeng merged commit 6a211af into Azure:master Oct 21, 2019
@jeremymeng jeremymeng deleted the imports-individual-model-types branch October 21, 2019 04:53
@ramya-rao-a ramya-rao-a mentioned this pull request Oct 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bterlson bterlson bterlson approved these changes

@XiaoningLiu XiaoningLiu XiaoningLiu approved these changes

@HarshaNalluru HarshaNalluru HarshaNalluru approved these changes

@jiacfan jiacfan jiacfan approved these changes

@vinjiang vinjiang Awaiting requested review from vinjiang

Assignees
No one assigned
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jeremymeng @bterlson @ramya-rao-a @XiaoningLiu @HarshaNalluru @jiacfan