[OSCI][Enhancement] ts cleanup and precommit

[kohinoor98]

Description

This PR contains all the eslint changes. This PR is broken down to 3 parts:

1. ESLint File Changes [OSCI][Enhancement] ts cleanup and precommit #930
2. lint checker for github actions [OSCI][Enhancement] lint checker for github actions #932
3. precommit(husky) and tsconfig changes [OSCI][Enhancement] precommit(husky) and tsconfig changes #931

Issues Resolved

#86

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[Hailong-am]

Is there any reason to update all license header? There is a guide for license header https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md#license-headers

[kohinoor98]

Hi @Hailong-am

I had referred some code in the parent repo and taken the header from there. Should I change it as per this(https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md#license-headers) guide?

[Hailong-am]

Hi @Hailong-am

I had referred some code in the parent repo and taken the header from there. Should I change it as per this(https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md#license-headers) guide?

Seems that both short and expanded version are ok to use opensearch-project/.github#98

[SuZhou-Joe]

It's quite a large change so I have some recommendations here:

• Split the PR into some smaller PRs like: workflow change / license header changes and corresponding source code format(If the license has to be changed to expanded one) / eslint rules change and corresponding source code format so that reviewers can safely approve the PR by only looking into changes on rules or license headers.
• Workflow failed for some reason and we have to fix all the failed actions.

[bowenlan-amzn]

@kohinoor98 really appreciate the effort to improve the quality of this code base

For the warning disabled, can you categorize and summarize those, so to decide what to do next, whether create TODO or fix this time or keep it disabled. Thanks.

[bowenlan-amzn]

Do we still need this git add here?

[kohinoor98]

For lint-staged v10 onwards, we do not need to add git add
We are using:

    "lint-staged": "^10.2.0",

It should be fine. Do you think I should add it as a fail-safe?

[bowenlan-amzn]

Good to know 👍

[kohinoor98]

It's quite a large change so I have some recommendations here:

• Split the PR into some smaller PRs like: workflow change / license header changes and corresponding source code format(If the license has to be changed to expanded one) / eslint rules change and corresponding source code format so that reviewers can safely approve the PR by only looking into changes on rules or license headers.
• Workflow failed for some reason and we have to fix all the failed actions.

Hi @SuZhou-Joe

Agreed! I have created to additional PRs for this issue to break down the current PR:
#931
#932

Thanks,
KC

[kohinoor98]

@kohinoor98 really appreciate the effort to improve the quality of this code base

For the warning disabled, can you categorize and summarize those, so to decide what to do next, whether create TODO or fix this time or keep it disabled. Thanks.

Hi @bowenlan-amzn

The eslint-disable-next-line and eslint-disable [message] indicate that the lint issues should be fixed or left based on the developer's instinct.
Should I incorporate TODOs or leave this as is for now, as the changes would require me to redo manual changes in ~100 files?

Thanks,
KC

[bowenlan-amzn]

@kohinoor98 I tried to get a list of all eslint-disable below. It's fine that we merge in this one w/o fixing any eslint-disable because the code has already been in production and we know it's working fine 👻.

Having this list could be put into a good first issue, and I believe there will be some learning when people reducing these disables.

/* eslint-disable jsx-a11y/click-events-have-key-events */
/* eslint-disable max-classes-per-file */
/* eslint-disable no-shadow */
/* eslint-disable prefer-const */
// eslint-disable-next-line @typescript-eslint/ban-types
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
// eslint-disable-next-line @typescript-eslint/explicit-member-accessibility
// eslint-disable-next-line @typescript-eslint/naming-convention
// eslint-disable-next-line @typescript-eslint/no-empty-interface
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
// eslint-disable-next-line @typescript-eslint/no-var-requires
// eslint-disable-next-line cypress/no-unnecessary-waiting
// eslint-disable-next-line jest/no-identical-title
// eslint-disable-next-line jsx-a11y/click-events-have-key-events
// eslint-disable-next-line no-empty
// eslint-disable-next-line no-shadow
// eslint-disable-next-line no-throw-literal
// eslint-disable-next-line no-unsafe-finally
// eslint-disable-next-line react-hooks/exhaustive-deps
// eslint-disable-next-line react/prefer-stateless-function

find cypress models public server test utils -type f -exec grep -rh "eslint-disable" {} + | sed 's/.*\/\/ eslint-disable/\/\/ eslint-disable/' | sort | uniq | sort

[kohinoor98]

@kohinoor98 I tried to get a list of all eslint-disable below. It's fine that we merge in this one w/o fixing any eslint-disable because the code has already been in production and we know it's working fine 👻.

Having this list could be put into a good first issue, and I believe there will be some learning when people reducing these disables.

/* eslint-disable jsx-a11y/click-events-have-key-events */
/* eslint-disable max-classes-per-file */
/* eslint-disable no-shadow */
/* eslint-disable prefer-const */
// eslint-disable-next-line @typescript-eslint/ban-types
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
// eslint-disable-next-line @typescript-eslint/explicit-member-accessibility
// eslint-disable-next-line @typescript-eslint/naming-convention
// eslint-disable-next-line @typescript-eslint/no-empty-interface
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
// eslint-disable-next-line @typescript-eslint/no-var-requires
// eslint-disable-next-line cypress/no-unnecessary-waiting
// eslint-disable-next-line jest/no-identical-title
// eslint-disable-next-line jsx-a11y/click-events-have-key-events
// eslint-disable-next-line no-empty
// eslint-disable-next-line no-shadow
// eslint-disable-next-line no-throw-literal
// eslint-disable-next-line no-unsafe-finally
// eslint-disable-next-line react-hooks/exhaustive-deps
// eslint-disable-next-line react/prefer-stateless-function

find cypress models public server test utils -type f -exec grep -rh "eslint-disable" {} + | sed 's/.*\/\/ eslint-disable/\/\/ eslint-disable/' | sort | uniq | sort

Yup 😂😂 @bowenlan-amzn

Even if all the disable lines exist, there should not be a problem, and someone could absolutely take it up as a good first issue to start about the codebase.

Please let me know what I could do to merge this PR in! I believe this PR would need to be revised/merged in with care as new files are added that conflict with files in this PR!

Thanks,
KC

[bowenlan-amzn]

@kohinoor98 please resolve the conflict. and we can merge in one PR with #931 , I just closed #932
Thanks for the effort!

[kohinoor98]

@kohinoor98 please resolve the conflict. and we can merge in one PR with #931 , I just closed #932 Thanks for the effort!

Hi @bowenlan-amzn

I have merged the two PRs. I have excluded @osd/eslint/no-restricted-paths rule. Please let me know if you want me to add it.

Cheers,
KC

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (be76a29) 63.37% compared to head (362d3ab) 63.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
+ Coverage   63.37%   63.40%   +0.03%     
==========================================
  Files         341      341              
  Lines       11554    11606      +52     
  Branches     2243     2115     -128     
==========================================
+ Hits         7322     7359      +37     
- Misses       3658     3662       +4     
- Partials      574      585      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.