[Issue 17826] Fix ES Lint errors in common\tools\dev\tool

[WeiJun428]

This fixes #17826 for common\tools\dev-tool.

I basically fixed errors such as missing return type, unused escape character, etc. for ts files in common\tools\dev-tool.

However, I didn't manage to fix Forbidden non-null assertion warnings in src\commands\samples\publish.ts and src\util\testProxyUtils.ts. I try to mute the warning by doing type checking object !== undefined according to TSLint but to not avail. May I get help on this?

I will remove the required snippet -f html -o <package name>-lintReport.html || exit 0 from package.json once the above-mentioned warning is resolved.

[ghost]

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

[ghost]

Thank you for your contribution, WeiJun428!

[jeremymeng]

However, I didn't manage to fix Forbidden non-null assertion warnings in src\commands\samples\publish.ts and src\util\testProxyUtils.ts. I try to mute the warning by doing type checking object !== undefined according to TSLint but to not avail. May I get help on this?

@witemple-msft @HarshaNalluru

[jeremymeng]

+@timovv who also worked on some of this.

[timovv]

Hey @WeiJun428, thanks for the contribution.

Regarding the non-null assertion warnings, as you suggest, checking that the object in question is not undefined and then removing the now redundant non-null assertion should be sufficient to stop the lint warning from showing up in both of those cases.

I did a bit of work in this area (before I noticed your PR) you can use -- for the case in samples/publish.ts, it should be sufficient to skip that entry if the file is undefined, like is done here. For the other case in testProxyUtils.ts, I refactored this method in a way that avoids the non-null assertion. Let me know if you have any questions on either of those.

[WeiJun428]

Hi @timovv. Thank you for your help to fix the warning!

[WeiJun428]

My PR is not passing the test; specifically, it fails the node unit test where the transpiled getConfigurationSetting.ts has slightly different spacing. I can't really find this file in the directory and of course didn't make any changes to this file. Can I get help on this too?

[timovv]

Thanks for integrating those changes, @WeiJun428! What you have looks good; I just have a couple of small things that would be nice to sort out. The test failure is related to the use of the dynamic import vs standard require -- see my inline comment for details.

[WeiJun428]

Thanks! @timovv. I learn a lot from your suggestions. I have decided to import the defaultPrettierOptions in common/tools/dev-tool/src/commands/samples/tsToJs.ts as I am unsure on how to suppress the error. It is fixed now, but may I know how would you suppress the error? Is it like adding a configuration comments in the file? Thank you!

[timovv]

Thanks for doing that. I can't see any issue with using import in the way you've done, but would want a second opinion -- @deyaaeldeen, I believe you had concerns that the behavior might differ between the ES6 import syntax and require in this case?

In terms of suppressing the lint error, an eslint-disable-next-line directive above the offending line would do the trick, e.g.:

{
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  ...(require(/* ... */) as prettier.Options),
  // ...
}

[deyaaeldeen]

@timovv that's right, we want to import the file at runtime in this case

@WeiJun428 please follow @timovv's suggestion to suppress the warning for require instead of using import.

[deyaaeldeen]

why is this type cast needed?

[WeiJun428]

Yeah. I have fixed it. It is redundant after I added AsyncGenerator<string> to common/tools/dev-tool/src/util/findMatchingFiles.ts

[deyaaeldeen]

Looks good! Thanks for your valuable contribution!

I left one minor comment.

[check-enforcer]

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

[timovv]

Looks great!

[jeremymeng]

Thanks @WeiJun428 for the contribution!