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 search packages #10780

Closed
praveenkuttappan opened this issue Aug 23, 2020 · 14 comments
Closed

Fix ESLint errors for search packages #10780

praveenkuttappan opened this issue Aug 23, 2020 · 14 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. help wanted This issue is tracking work for which community contributions would be welcomed and appreciated Search

Comments

@praveenkuttappan
Copy link
Member

praveenkuttappan commented Aug 23, 2020

Fix lint errors found in search packages by ESLint. Following are the steps to run ESLint for search packages and reproduce this issue.

  1. Set up your dev environment if not already done so as mentioned here
  2. Go to <repo root>/sdk/search/<package-name>
  3. run rushx lint
  4. Command in step 2 generates an html report in directory <repo root>/sdk/search/<package-name> with name 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 in package root <repo root>/sdk/search /<package-name> to treat any new lint regression as hard failure in CI.

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

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

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 23, 2020
@ramya-rao-a ramya-rao-a added this to the Backlog milestone Aug 25, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 25, 2020
@ramya-rao-a ramya-rao-a added Client This issue points to a problem in the data-plane of the library. Search labels Aug 25, 2020
@ramya-rao-a ramya-rao-a added help wanted This issue is tracking work for which community contributions would be welcomed and appreciated and removed Up for grabs labels Sep 15, 2020
@abc516
Copy link
Contributor

abc516 commented Oct 6, 2020

Hi @praveenkuttappan , is this still an open issue? I'm willing to take it on

@praveenkuttappan
Copy link
Member Author

This still seems like an open issue. Adding @deyaaeldeen and @ramya-rao-a to confirm,

@deyaaeldeen
Copy link
Member

@abc516 @praveenkuttappan Yes this is still an issue, here is the linting report.

@abc516
Copy link
Contributor

abc516 commented Oct 6, 2020

ok @deyaaeldeen @praveenkuttappan can you assign this to me please?

@ramya-rao-a
Copy link
Contributor

@abc516 Please go ahead and submit a PR for fixing the linter errors

@abc516
Copy link
Contributor

abc516 commented Oct 13, 2020

Hi @deyaaeldeen @ramya-rao-a @praveenkuttappan I'm working on this issue atm. I'm running into a lot of lint errors regarding unit tests (no-useless catch). According to the rule on eslint (https://eslint.org/docs/rules/no-useless-catch), I could delete the catch block but I feel that would mess up the logic. I'd like some input before I proceed, please. Thanks. Here's a snippet from the searchIndexClient.spec.ts file below.

  try {
    synonymMap = await indexClient.getSynonymMap("my-azure-synonymmap-3");
    assert.equal(synonymMap.name, "my-azure-synonymmap-3");
    assert.equal(synonymMap.synonyms.length, 2);
    const synonyms = [
      "United States, United States of America => USA",
      "Washington, Wash. => WA"
    ];
    assert.include(synonyms, synonymMap.synonyms[0]);
    assert.include(synonyms, synonymMap.synonyms[1]);
  } catch (ex) {
    throw ex;
  } finally {
    await indexClient.deleteSynonymMap(synonymMap);
  }

@ramya-rao-a
Copy link
Contributor

In the above code snippet, I dont see how removing the catch block would change the logic. The finally block would still work even if we don't have the explicit catch block which does nothing but rethrow the error

@sarangan12, Do you recall the reason you had the above catch block?

@sarangan12
Copy link
Contributor

Originally, I thought I can do some retry in the catch. Later, decided they were not necessary. I am Ok with removing the catch block.

@abc516
Copy link
Contributor

abc516 commented Oct 16, 2020

OK @sarangan12 I made a PR for this. Can you add the label hacktoberfest-accepted to the PR please?

@ramya-rao-a
Copy link
Contributor

@abc516 This repository has the hacktoberfest topic and so the individual PRs will not need the hacktoberfest-accepted label

@ramya-rao-a
Copy link
Contributor

@sarangan12, @xirzec, The pending linting errors and warning are:

image

In #11868, we discussed about disabling

  • the rule ts-naming-options for SearchClientOptions
  • no-unused-expression for interval?.unref()

How do you want to tackle the two warnings about classes being passed to methods instead of interfaces?

@xirzec
Copy link
Member

xirzec commented Oct 22, 2020

I think we should replace the class reference with an interface that is compatible with whatever searchIndexingBufferedSender actually needs to call.

@ramya-rao-a ramya-rao-a modified the milestones: Backlog, MQ-2020 Nov 16, 2020
@ramya-rao-a ramya-rao-a assigned sarangan12 and unassigned deyaaeldeen Jan 4, 2021
@ramya-rao-a ramya-rao-a removed this from the MQ-2020 milestone Jan 4, 2021
@sarangan12
Copy link
Contributor

Code complete with PR: #13114

@ramya-rao-a
Copy link
Contributor

Yay!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 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. help wanted This issue is tracking work for which community contributions would be welcomed and appreciated Search
Projects
None yet
Development

No branches or pull requests

6 participants