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

163 review linting implementation and resolve errors #164

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

samwarwick
Copy link
Contributor

This PR addresses items 1-4 of the AC in #163. Resolving all linting errors is not a trivial task and should be addressed separately. I feel there is value in merging this PR now so that we can establish a baseline for progression.

ESLint has been added to package.json and can be run with npm run lint. Note that warnings are currently disabled.

The original TSlint rules (tslint.json) were converted to ESLint automatically using tslint-to-eslint-config. tslint.json has been left in the repo for now for comparison purposes but can eventually be removed.

In an attempt to get the baseline reporting as close to TSLint as possible these additional rules were disabled:

"@typescript-eslint/naming-convention": "off",
"@typescript-eslint/indent": "off",
"indent": "off",
"no-underscore-dangle": "off",
"@typescript-eslint/ban-ts-comment": "off",
"@typescript-eslint/ban-types": "off",
"prefer-const": "off",

Despite this, there are still 692 ESLint errors, compared to 444 with TSLint. Over half of them are code-formatting issues which can easily be resolved with eslint --fix. Executing a dry-run with npx eslint . --quiet --fix-dry-run indicates there are still 153 which require manual resolution.

Two code files have been modified to get the linting passing for illustration purposes. e.g. npx eslint src/core/index.ts --quiet

Before making any further code changes to satisfy the linter, I suggest:

  1. Having a broader team discussion on what baseline rules to apply. The original TSLint rules are probably out-dated. Consistency across other TokenScript repos also needs consideration.
  2. Reviewing the test coverage (e.g. Token Negotiator Unit Tests Uplift #156) to have greater confidence in code changes.

@samwarwick samwarwick linked an issue Apr 19, 2022 that may be closed by this pull request
@sonarcloud
Copy link

sonarcloud bot commented Apr 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@nicktaras
Copy link
Collaborator

This is a great way forward to use ESLint instead of TSLint, thanks Sam.

  1. Agree. Let's catch up to look at this.
  2. Also sounds good.

I'll set up a meeting for tomorrow.

@nicktaras nicktaras merged commit f41d315 into main Apr 20, 2022
@nicktaras nicktaras deleted the 163-review-linting-implementation-and-resolve-errors branch April 21, 2022 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review linting implementation and resolve errors
2 participants