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.

Sign up for GitHub

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

Jump to bottom

Migrate from tslint to eslint #2415

Merged
merged 5 commits into from
Sep 6, 2022
Merged

Migrate from tslint to eslint #2415

merged 5 commits into from
Sep 6, 2022

Conversation

FieteO
Copy link
Contributor

@FieteO FieteO commented Apr 15, 2022

This PR aims to replace tslint with eslint, since the former is deprecated since 2019.

@FieteO FieteO force-pushed the migrate-from-tslint-to-eslint branch from 1bffe4c to 94df177 Compare May 12, 2022 20:20
datho7561
datho7561 requested changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Looks like a promising start, thanks for the contribution! I just have a few suggestions that might make the eslint setup a bit more helpful. As well, I found a few linting errors in the codebase after I rebased this PR.

.vscode/tasks.json Outdated Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rgrunber rgrunber added this to the End August 2022 milestone Jul 26, 2022
@datho7561
Copy link
Contributor

I'm going to push a commit onto this PR in order to address the conflicts and the feedback I had...

@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch from 94df177 to 9560b1e Compare August 15, 2022 19:47
@datho7561 datho7561 requested a review from rgrunber August 15, 2022 19:47
@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch from 9560b1e to 91c4150 Compare August 15, 2022 19:51
@FieteO
Copy link
Contributor Author

FieteO commented Aug 16, 2022

Sorry for not giving feedback @datho7561. I didn't really have time to check what my reasoning was :D As far as I can remember including the custom arrow plugin and not including the recommended rules was to recreate the tslint status-quo as close as possible.

@datho7561
Copy link
Contributor

datho7561 commented Aug 16, 2022 •
edited
Loading

Sorry for not giving feedback @datho7561. I didn't really have time to check what my reasoning was :D

No worries!

As far as I can remember including the custom arrow plugin and not including the recommended rules was to recreate the tslint status-quo as close as possible.

Okay. That makes sense. Using the recommended rules meant that I needed to refactor a large portion of the codebase to fix lint errors, so it might make sense to stick with what you had. @rgrunber, what do you think about this?

@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch from 91c4150 to c04efd2 Compare August 16, 2022 19:59
@datho7561
Copy link
Contributor

I removed the part where I inherited the default eslint configuration and the refactoring I did to adapt to this, but left in switching from the extension for arrow functions to the default eslint arrow function functionality.

@datho7561
Copy link
Contributor

Taking a look at it, I don't think eslint is being run on TypeScript files as-is. If I run npx eslint --ext .js,.ts ., then I get a bunch of naming errors in the typescript files.

@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch 2 times, most recently from 13d4e3f to f73c7f8 Compare August 23, 2022 13:39
@rgrunber rgrunber removed this from the End August 2022 milestone Aug 31, 2022
@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch 3 times, most recently from 7ca67cc to b7ebbef Compare September 2, 2022 19:25
@datho7561
Copy link
Contributor

@rgrunber I just pushed a commit that makes sure that the webpack watch task works correctly (VS Code waits until webpack finishes bundling the extension before launching)

@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch from b7ebbef to 3f5893b Compare September 2, 2022 20:08
@rgrunber rgrunber added this to the End September milestone Sep 6, 2022
@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch from 3f5893b to b094ade Compare September 6, 2022 20:35
@datho7561
Fix problem matcher
3b5898a 
Signed-off-by: David Thompson <[email protected]>
@datho7561 datho7561 force-pushed the migrate-from-tslint-to-eslint branch from b094ade to 3b5898a Compare September 6, 2022 20:56
rgrunber
rgrunber approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

I've tested this out and it works for me. With the installed recommended problem matchers we even wait for the compilation to complete, which is nice.

The linting is quite a bit more strict, but that may be for the best.

@rgrunber rgrunber merged commit d6a2fd5 into redhat-developer:master Sep 6, 2022
gayanper pushed a commit to gayanper/vscode-java that referenced this pull request Sep 8, 2022
@FieteO @datho7561 @gayanper
Migrate from tslint to eslint (redhat-developer#2415)
75e11b9 
* Add eslint
* Remove tslint references, replace tslint ignores with eslint ignores
* Ignore node_modules, use eslint arrow function, add webpack plugin
* Lint ts files, address lint errors in ts files
* Fix problem matcher

Signed-off-by: Fiete Ostkamp <[email protected]>
Co-authored-by: David Thompson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datho7561 datho7561 datho7561 requested changes

@rgrunber rgrunber rgrunber approved these changes

Assignees
No one assigned
Labels
build/infra
Projects
None yet
Milestone
End September 2022
Development

Successfully merging this pull request may close these issues.
3 participants
@FieteO @datho7561 @rgrunber