Skip to content
This repository has been archived by the owner on Nov 13, 2023. It is now read-only.

Migrate from TSLint to ESLint #652

Merged
merged 26 commits into from
Aug 17, 2021
Merged

Migrate from TSLint to ESLint #652

merged 26 commits into from
Aug 17, 2021

Conversation

t1m0thyj
Copy link
Member

@t1m0thyj t1m0thyj commented Aug 5, 2021

ℹ️ This PR is easier to review with whitespace changes hidden. It will still be a big diff though 😢
⚠️ In cases where tests were removed, it is because they were identified as duplicates by ESLint.

TODO:

  • Discuss lint rules with the team
  • Fix or disable remaining lint errors
  • Fix or disable remaining lint warnings
  • Remove comments that disable TSLint rules
  • Remove TSLint from the test CLIs
  • Update/remove Gulp task that uses TSLint

t1m0thyj added 13 commits August 5, 2021 09:06
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@t1m0thyj
Copy link
Member Author

t1m0thyj commented Aug 9, 2021

TSLint Rule ESLint Equivalent Is Migrated
no-submodule-imports none, but import/no-internal-modules is similar [1]
max-line-length = 150 max-len ✔️
no-magic-numbers yes, no-magic-numbers but I don't like it
no-consecutive-blank-lines no-multiple-empty-lines ✔️
indent = 4 spaces indent ✔️
whitespace = check-module none, but object-curly-spacing is similar [2]
trailing-comma (in zowe-cli) yes, comma-dangle but @dkelosky doesn't like it [3]

[1] It prevents importing submodules from inside your own project, but we want to prevent importing them from other packages.
[2] It enforces spacing for curly braces everywhere, but we want to only enforce it for import/export statements.
[3] Alternatively, we could allow trailing commas for multiline objects, and forbid them on single line objects.

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@t1m0thyj
Copy link
Member Author

t1m0thyj commented Aug 9, 2021

After discussing with the team:

  • no-submodule-imports - ok to ignore
  • no-magic-numbers - add for production code, but ignore in tests
  • whitespace - ok to ignore
  • trailing-comma - add for multiline, but ignore for single line

@t1m0thyj t1m0thyj marked this pull request as ready for review August 9, 2021 20:39
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #652 (5236109) into master (385e698) will increase coverage by 0.00%.
The diff coverage is 90.74%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #652   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files         163      163           
  Lines        7891     7880   -11     
  Branches     1375     1375           
=======================================
- Hits         6501     6492    -9     
+ Misses       1386     1384    -2     
  Partials        4        4           
Impacted Files Coverage Δ
packages/cmd/src/CommandPreparer.ts 96.82% <ø> (ø)
packages/cmd/src/help/HelpGeneratorFactory.ts 60.00% <ø> (ø)
packages/cmd/src/help/WebHelpManager.ts 93.75% <ø> (ø)
.../src/help/abstract/AbstractHelpGeneratorFactory.ts 100.00% <ø> (ø)
packages/cmd/src/response/HandlerResponse.ts 63.63% <ø> (ø)
packages/cmd/src/yargs/AbstractCommandYargs.ts 36.97% <0.00%> (+0.30%) ⬆️
packages/cmd/src/yargs/CommandYargs.ts 39.58% <0.00%> (ø)
packages/cmd/src/yargs/GroupCommandYargs.ts 33.33% <ø> (ø)
packages/constants/src/Constants.ts 100.00% <ø> (ø)
packages/imperative/src/ConfigurationValidator.ts 70.73% <ø> (ø)
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df4e727...5236109. Read the comment docs.

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespaces certainly reduces the number of lines changed 😋
Still takes a little while to review.
LGTM though! 😉

@@ -9,6 +9,7 @@
*
*/

/* eslint-disable jest/expect-expect */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might be able to rename the functions (like wasNpmInstallCallValid) which contain the expects to something like expectNpmInstallCallValid (or soemthing to that matches the assertFunctionNames) 😋

Copy link
Member

@awharn awharn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple questions before I approve this.

@@ -9,14 +9,6 @@
*
*/

import { IImperativeConfig } from "../../../../../packages/imperative";
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it supposed to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not sure how this happened, thanks for catching it!

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@t1m0thyj t1m0thyj linked an issue Aug 17, 2021 that may be closed by this pull request
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@t1m0thyj t1m0thyj merged commit e527067 into master Aug 17, 2021
@t1m0thyj t1m0thyj deleted the use-eslint branch August 17, 2021 16:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace the soon-to-be-deprecated TSLint with ESLint
3 participants