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

Fix ESLint errors for internal tool packages #17826

Closed
5 of 8 tasks
jeremymeng opened this issue Sep 23, 2021 · 4 comments · Fixed by #18819
Closed
5 of 8 tasks

Fix ESLint errors for internal tool packages #17826

jeremymeng opened this issue Sep 23, 2021 · 4 comments · Fixed by #18819
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. eslint plugin help wanted This issue is tracking work for which community contributions would be welcomed and appreciated

Comments

@jeremymeng
Copy link
Member

jeremymeng commented Sep 23, 2021

Nice to have
The following internal tools/tests still ignore ESLint errors.

  • common\tools\dev-tool
  • sdk\eventhub\mock-hub
  • sdk\test-utils\recorder-new
  • sdk\test-utils\testing-recorder-new
  • sdk\test-utils\perfstress
  • sdk\storage\perf-tests\storage-blob-track-1
  • sdk\test-utils\recorder
  • sdk\storage\perf-tests\storage-file-share-track-1

We have special eslint configuration file to be used to lint internal packages: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/.eslintrc.internal.json

An example of using it can be found in Text Analytics perf tests.

We should fix ESLint errors for each of them. Feel free to send separate PRs for each package. Following are the steps to run ESLint for these packages and reproduce this issue.

  1. Set up your dev environment if not already done so as mentioned here
  2. Go to any of the folders above
  3. run rushx lint
  4. Command in step 2 generates an html report in the same folder with name that ends with lintReport.html
  5. All lint errors found in this package is listed on html report.

Once all known issues are resolved, below change is required in package.json file the same folder to treat any new lint regression as hard failure in CI.

  • Remove following snippet from lint command in package.json
    -f html -o <package name>-lintReport.html || exit 0

Note: HTML report name prefix may be different for each package name to differentiate the report for each package.

@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system. labels Sep 23, 2021
@jeremymeng jeremymeng added this to the [2021] November milestone Sep 23, 2021
@jeremymeng jeremymeng changed the title Fix ESLint errors for remaining packages Fix ESLint errors for internal tool packages Sep 28, 2021
@jeremymeng jeremymeng added Epic eslint plugin and removed EngSys This issue is impacting the engineering system. eslint plugin Epic labels Sep 29, 2021
@ramya-rao-a ramya-rao-a added the help wanted This issue is tracking work for which community contributions would be welcomed and appreciated label Oct 1, 2021
@ramya-rao-a ramya-rao-a modified the milestones: [2021] November, Backlog Oct 1, 2021
@ramya-rao-a ramya-rao-a modified the milestones: Backlog, [2022] January Nov 2, 2021
@timovv
Copy link
Member

timovv commented Dec 1, 2021

Reopening this for the moment since there are still a few more packages to go.

@timovv timovv reopened this Dec 1, 2021
timovv added a commit to timovv/azure-sdk-for-js that referenced this issue Dec 13, 2021
@deyaaeldeen
Copy link
Member

We have special eslint configuration file to be used to lint internal packages: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/.eslintrc.internal.json

An example of using it can be found in Text Analytics perf tests.

@timovv
Copy link
Member

timovv commented Dec 16, 2021

The track 1 perf tests aren't currently built by CI, so enabling the linter in this way won't do anything. And I believe we're planning on combining the recorder-new package and the old recorder package sometime in the near future, so it may be worth holding off on enabling the linter until that stage. I think this issue can now be closed, unless there's any objection.

@ramya-rao-a
Copy link
Contributor

Sounds good to me

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. eslint plugin help wanted This issue is tracking work for which community contributions would be welcomed and appreciated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants