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

[Linter] Added sort-imports rule with "warning" #18730

Merged
merged 4 commits into from
Nov 22, 2021

Conversation

bzhang0
Copy link
Contributor

@bzhang0 bzhang0 commented Nov 18, 2021

This PR is working in conjunction with other (upcoming) PRs to solve #9252.
As per my previous PR #18598, I will be making multiple PRs to fix individual sections of the azure codebase with respect to the new sort-imports rule.

In this PR, I have simply added sort-imports in common/tools/eslint-plugin-azure-sdk/.eslintrc.json and azure-sdk-for-js/common/tools/eslint-plugin-azure-sdk/src/configs/azure-sdk-base.ts with a 1 output (warning) or "warn" output (also warning).

@ghost ghost added eslint plugin customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Nov 18, 2021
@ghost
Copy link

ghost commented Nov 18, 2021

Thank you for your contribution bzhang0! We will review the pull request and get back to you soon.

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 18, 2021

Should I wait for this PR to be merged in before starting another branch to fix related directories? I remember last time in this pr (#18567) I had duplicated code in both branches (from main - this is not the exact case this time but it's relatively similar).

Since this commit changes the .eslintrc.json and azure-sdk-base.ts files, I was wondering if I could do the same thing in another branch and begin calling git cherry-pick on my past commits.

Thanks!

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 18, 2021

I'm seeing the following check failing.

==[ FAILURE: 1 project ]=======================================================

--[ FAILURE: @azure/monitor-query ]----------------[ 3 minutes 58.4 seconds ]--

ESLint found too many warnings (maximum: 0).


rush lint (5 minutes 53.6 seconds)
Projects failed to build.

image

I'm not exactly sure why this is appearing - I only added two lines and made them output with a warning as well. Could I possibly get some help regarding this issue?

@jeremymeng
Copy link
Member

Should I wait for this PR to be merged in before starting another branch to fix related directories? I remember last time in this pr (#18567) I had duplicated code in both branches (from main - this is not the exact case this time but it's relatively similar).

You could create new branches based on this PR branch. The commit in the PR will be included in your other PRs too but it should be fine after this PR is merged as there isn't any conflict. There is a way to make other PRs cleaner after this PR is merged (using git rebase --onto ...) but it's not really that important.

@jeremymeng
Copy link
Member

@bzhang0 the monitor-query error is probably because that package has additional linting option --max-warnings 0

"lint": "eslint package.json api-extractor.json src test --format unix --ext .ts --max-warnings 0",

could you remove it and see how it goes?

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 18, 2021

@jeremymeng yes! I will try this out

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 19, 2021

Deleted the section, now waiting for the tests...

Copy link
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.

Looks good!

@jeremymeng
Copy link
Member

Looking into the build failure

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 19, 2021

Looking into the build failure

It's possible I forgot to rush rebuild? I'm not sure how that would impact on the remote side though.

@jeremymeng
Copy link
Member

@bzhang0 could you please try merge the latest from upstream/main? I think there's some new changes just merged thus causing the diff between main and your PR in the eng/common folder.

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 20, 2021

@bzhang0 could you please try merge the latest from upstream/main? I think there's some new changes just merged thus causing the diff between main and your PR in the eng/common folder.

Alright! I will try

@jeremymeng jeremymeng merged commit 9f454f8 into Azure:main Nov 22, 2021
@bzhang0 bzhang0 deleted the linter-sort-imports branch December 5, 2021 11:03
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. eslint plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants