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

[Core lro] Update linting scripts and auto linting #11273

Merged
merged 6 commits into from
Sep 25, 2020

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Sep 15, 2020

Switching the eslint configuration file used for core-lro to use the newer one in the root of the repository and fixing all the linting problems. Furthermore, enforce the absence of linting errors in CI.

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.

A few concerns

sdk/core/core-lro/package.json Outdated Show resolved Hide resolved
sdk/core/core-lro/src/poller.ts Outdated Show resolved Hide resolved
sdk/core/core-lro/src/shims.d.ts Outdated Show resolved Hide resolved
sdk/core/core-lro/test/utils/testOperation.ts Show resolved Hide resolved
Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Some comments below. Note for the comment about the this concerns, I'm not necessarily saying that we should block this PR for that, just to figure out if we're going to leave it as is or make a change and then we could take an issue on that change if we do decide that.

sdk/core/core-lro/package.json Outdated Show resolved Hide resolved
sdk/core/core-lro/package.json Outdated Show resolved Hide resolved
sdk/core/core-lro/test/utils/testOperation.ts Show resolved Hide resolved
Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Hi! Sorry for rejecting this PR. I love the idea! Jeff's feedback are essential though. Take your time!

@sadasant
Copy link
Contributor

@deyaaeldeen PR description please 🙏✨

I know we miss it in many places, but we can work together on improving 💪

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Make sure CI passes and we're good 👏

@deyaaeldeen
Copy link
Member Author

@sadasant Thanks for your feedback. The CI fails because #11403 and #11411 are not merged yet.

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Concerned about removal of dist-esm. Let's talk about this offline. CC @xirzec, I left a comment on #11411

@deyaaeldeen deyaaeldeen merged commit 0d5bd96 into Azure:master Sep 25, 2020
@deyaaeldeen deyaaeldeen deleted the core-lro-linting branch September 25, 2020 20:43
deyaaeldeen added a commit to deyaaeldeen/azure-sdk-for-js that referenced this pull request Sep 25, 2020
* [Core lro] Update linting scripts and auto linting

* applying formatting changes

* address Jeff's feedback

* remove dist-esm/**

* adding dist-esm/src back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants