Skip to content

Moves Aborter class into separate package#3266

Merged
chradek merged 11 commits into
Azure:masterfrom
chradek:feature/aborter-package
Jun 6, 2019
Merged

Moves Aborter class into separate package#3266
chradek merged 11 commits into
Azure:masterfrom
chradek:feature/aborter-package

Conversation

@chradek
Copy link
Copy Markdown
Contributor

@chradek chradek commented May 29, 2019

This PR builds on #2373 and moves the Aborter into its own package. Once this makes it into master we can update existing packages to use the Aborter from this package.

@chradek chradek force-pushed the feature/aborter-package branch from 4762287 to ffa82b3 Compare May 30, 2019 21:03
@chradek chradek assigned jeremymeng and unassigned jeremymeng May 30, 2019
@chradek chradek requested a review from jeremymeng May 30, 2019 22:41
jeremymeng
jeremymeng reviewed May 30, 2019
View reviewed changes
Comment thread sdk/core/aborter/tsconfig.json Outdated
Copy link
Copy Markdown
Member

@jeremymeng jeremymeng May 30, 2019

Choose a reason for hiding this comment

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

I believe we now prefer types over typings.

Copy link
Copy Markdown
Contributor Author

@chradek chradek May 31, 2019

Choose a reason for hiding this comment

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

Updated to use types instead of typings.

jeremymeng
jeremymeng reviewed May 30, 2019
View reviewed changes
Comment thread sdk/core/aborter/.prettierrc.json Outdated
Copy link
Copy Markdown
Member

@jeremymeng jeremymeng May 30, 2019

Choose a reason for hiding this comment

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

We should use the existing sdk/.prettierrc.json

Copy link
Copy Markdown
Contributor Author

@chradek chradek May 31, 2019

Choose a reason for hiding this comment

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

Removed .prettierrc.json from the core/aborter package and now using the one located under sdk. Ran check-format and format scripts and committed those changes as well.

jeremymeng
jeremymeng reviewed May 30, 2019
View reviewed changes
Comment thread sdk/core/aborter/package.json Outdated
Copy link
Copy Markdown
Member

@jeremymeng jeremymeng May 30, 2019

Choose a reason for hiding this comment

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

../../.prettierrc.json

Copy link
Copy Markdown
Contributor Author

@chradek chradek May 31, 2019

Choose a reason for hiding this comment

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

Updated location in check-format and format. Thanks!

jeremymeng
jeremymeng reviewed May 31, 2019
View reviewed changes
Comment thread sdk/core/aborter/src/aborter.ts Outdated
Copy link
Copy Markdown
Member

@jeremymeng jeremymeng May 31, 2019

Choose a reason for hiding this comment

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

We might need to update examples as blockBlobClient is storage specific.

Copy link
Copy Markdown
Contributor Author

@chradek chradek May 31, 2019

Choose a reason for hiding this comment

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

That's fair, I think I should be able to adapt the examples in the readme to work for this documentation as well.

Copy link
Copy Markdown
Contributor Author

@chradek chradek May 31, 2019

Choose a reason for hiding this comment

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

Updated to be similar to the examples in the README.

jeremymeng
jeremymeng requested changes May 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Some minor comments. Otherwise looks good.

@jeremymeng
Copy link
Copy Markdown
Member

jeremymeng commented May 31, 2019

Please check with @bterlson about the need to invetigate doc impact in the original PR.

@chradek chradek force-pushed the feature/aborter-package branch from ffa82b3 to 0a21ac3 Compare May 31, 2019 16:37
@chradek
Copy link
Copy Markdown
Contributor Author

chradek commented Jun 3, 2019

@bterlson
Do you have any concerns on the documentation impact of this change? This PR does not include updating existing packages (e.g. storage-*) to consume this package.

@bterlson
Copy link
Copy Markdown
Member

bterlson commented Jun 3, 2019

As nothing has been updated to use the dependency yet, the documentation impact seems zero. But once we update a project's dependencies, we will want to check that the docs render/link appropriately. The docs for this package may need to be onboarded, the docs teams channel may be able to help with that process.

@chradek
Copy link
Copy Markdown
Contributor Author

chradek commented Jun 3, 2019

@bterlson
I talked to @ramya-rao-a and she mentioned the docs are published after we publish the package to NPM. I was able to verify that I could build the documentation for this package locally.

@chradek chradek force-pushed the feature/aborter-package branch 2 times, most recently from 0ce1857 to ea0f777 Compare June 5, 2019 16:34
@chradek
Copy link
Copy Markdown
Contributor Author

chradek commented Jun 5, 2019

@jeremymeng
Now that CI issues are resolved, this should be ready for a final review. I've addressed your feedback, and made one additional change to rename the folder containing this package from sdk/core/aborter to sdk/core/core-aborter. This was done due to the feedback received for core-http.

jeremymeng
jeremymeng reviewed Jun 5, 2019
View reviewed changes
Comment thread sdk/core/core-aborter/api-extractor.json Outdated
Copy link
Copy Markdown
Member

@jeremymeng jeremymeng Jun 5, 2019

Choose a reason for hiding this comment

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

please add a line to disable generation of untrimmed dts rollup. otherwise it is generated by default into dist/ and thus gets packaged.

   "untrimmedFilePath": "",

See similar changes in https://github.com/Azure/azure-sdk-for-js/pull/3461/files

@chradek chradek force-pushed the feature/aborter-package branch from baa72dd to ee748ef Compare June 5, 2019 23:46
@chradek chradek merged commit 7a731ae into Azure:master Jun 6, 2019
@chradek chradek deleted the feature/aborter-package branch June 6, 2019 00:08
@ramya-rao-a ramya-rao-a mentioned this pull request Jun 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremymeng jeremymeng jeremymeng approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chradek @jeremymeng @bterlson