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

Migrate ESLint to new linting configration #842

Closed
4 of 10 tasks
seratch opened this issue Mar 22, 2021 · 5 comments · Fixed by #1024
Closed
4 of 10 tasks

Migrate ESLint to new linting configration #842

seratch opened this issue Mar 22, 2021 · 5 comments · Fixed by #1024
Assignees
Labels
discussion M-T: An issue where more input is needed to reach a decision good first issue Good for newcomers tests M-T: Testing work only
Milestone

Comments

@seratch
Copy link
Member

seratch commented Mar 22, 2021

Description

⚠️ TSLint has been deprecated as of 2019. Please see this issue for more details: Roadmap: TSLint → ESLint. typescript-eslint is now your best option for linting TypeScript.

https://palantir.github.io/tslint/

@stevengill @mwbrooks @misscoded
How can we move #669 forward?

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • example code related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@seratch seratch added discussion M-T: An issue where more input is needed to reach a decision good first issue Good for newcomers tests M-T: Testing work only labels Mar 22, 2021
@stevengill stevengill self-assigned this Mar 22, 2021
@seratch seratch added this to the 3.5.0 milestone Mar 23, 2021
@stevengill stevengill removed their assignment Jun 1, 2021
@seratch seratch modified the milestones: 3.5.0, 3.6.0 Jul 2, 2021
@filmaj
Copy link
Contributor

filmaj commented Jul 28, 2021

Pardon the dumb question, but it looks to me like we already use eslint in this repo? Is there something else that we'd like done in this repo to solve this issue?

EDIT: sounds like should review #669 to incorporate some of the linting rule updates and also 🪓 prettier.

@filmaj filmaj self-assigned this Jul 28, 2021
@filmaj
Copy link
Contributor

filmaj commented Jul 28, 2021

I have started looking at this issue. I have pulled out just the configuration changes originally proposed by Ankur in #669 and merged into a branch up to date with latest main. Looks like there may need to be many changes to the project in order to adhere to the rules Ankur converged on:

✖ 1532 problems (1342 errors, 190 warnings)
  187 errors and 0 warnings potentially fixable with the `--fix` option.

@seratch I noticed that in #669 you had recommended separating some of the changes out into a different pull request. Can you elaborate on your thinking here? I'm trying to understand the approach you'd like to use to land these changes to the linting in this project.

Perhaps one alternative approach is to open a base pull request with just the configuration changes, where we can review and debate those on their own (let's call this branch eslint). The assumption would be that this eslint branch would not pass CI initially. Then, separately, we could issue other pull requests, using the eslint branch as a base, to slowly and incrementally update the source files to adhere to the linting. With each such pull request, the number of lint failures would slowly drop. Eventually, once the eslint branch is updated with all source files adhering to the new linting rules, we can merge it into main.

Let me know what your thoughts are on this! I am looking forward to contributing to the team 💪

@seratch
Copy link
Member Author

seratch commented Jul 28, 2021

@filmaj

Can you elaborate on your thinking here? I'm trying to understand the approach you'd like to use to land these changes to the linting in this project.

I know it's inevitable if we turn the new settings on at a time but my impression of the pull request is that it has many changes that are unrelated to one another. Therefore, I thought that having multiple smaller pull requests (for instance, one for moving WebClientPool from App, another one for converting the complicated chains of cond2 ? condition1 ? A : B : C) would make the reviews much easier.

Perhaps one alternative approach is to open a base pull request with just the configuration changes, where we can review and debate those on their own (let's call this branch eslint).

I like this approach a lot. I was thinking that we can merge smaller new-eslint-setting-preparation pull requests before enabling the new configuration but your approach here is better.

@seratch seratch changed the title Migrate TSLint to ESLint Migrate ESLint to new linting configration Jul 28, 2021
@seratch
Copy link
Member Author

seratch commented Jul 28, 2021

Also, as you pointed out, we already use eslint in this repository. I've updated the issue title to be more accurate.

@filmaj
Copy link
Contributor

filmaj commented Jul 28, 2021

I like this approach a a lot. I was thinking that we can merge smaller new-eslint-setting-preparation pull requests before enabling the new configuration but your approach here is better.

Cool! This way we can also proof out how the configuration changes impact the code, and how it looks like, one file, or one rule, at a time, however we wish to divide the work up.

Also, as you pointed out, we already use eslint in this repository. I've updated the issue title to be more accurate.

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision good first issue Good for newcomers tests M-T: Testing work only
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants