-
Notifications
You must be signed in to change notification settings - Fork 77
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
Migrate from TSLint to ESLint #1250
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1250 +/- ##
==========================================
+ Coverage 54.84% 58.27% +3.43%
==========================================
Files 105 105
Lines 2859 2598 -261
Branches 503 291 -212
==========================================
- Hits 1568 1514 -54
- Misses 1030 1033 +3
+ Partials 261 51 -210 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this PR and it didn't catch some of the lint mistakes that I introduced. For exampl,e, i tried importing some unused components in a file, and the old linter caught it while the new linter passed all the files.
Perhaps it failed to convert the lint rules in tslint.json
to those in .eslintrc.json
?
…247-upgradeTSLint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel not all the lint changes are for the best. for example the changes in app-routing.module.ts
makes it harder to read.
Also another personal preference would be to not change code en masse for the sake of styling as it makes it harder to trace code changes.
please revert the changes in the repository, then check the arrow-body-style
rule again
.eslintrc.json
Outdated
"type": "attribute" | ||
} | ||
], | ||
"arrow-body-style": ["error", "always"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try changing this to as-needed
https://eslint.org/docs/latest/rules/arrow-body-style#as-needed
53b282f
to
ed8e3da
Compare
…Tcher into 1247-upgradeTSLint
src/app/app.module.ts
Outdated
return { headers: { Authorization: `Token ${this.authService.accessToken.getValue()}` } }; | ||
}); | ||
const basic = setContext(() => ({ headers: { Accept: 'charset=utf-8' } })); | ||
const auth = setContext(() => ({ headers: { Authorization: `Token ${this.authService.accessToken.getValue()}` } })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i think these changes are good for readability
The following comment blocks are valid: /**************** * Comment block ****************/ /**************** * Comment block */
5deb705
to
a243ae7
Compare
I cannot identify the issue. It is failing on github ci but not locally. On Github CI, it is identifying lint errors where it shouldnt be as well.
For some unknown reason, the One of the instances is below, in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this PR without the arrow-body-style rule for now.
I don't think it is worth the effort to figure out why it doesn't work for eslint v7 when official documentation only exists for eslint v8 and v9. We should revisit this when eslint is upgraded to its latest version.
Summary:
Fixes #1247
Changes Made:
Proposed Commit Message: