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

fix: Update eslint to v7 #8149

Closed
wants to merge 1 commit into from
Closed

fix: Update eslint to v7 #8149

wants to merge 1 commit into from

Conversation

RDIL
Copy link
Contributor

@RDIL RDIL commented Jul 8, 2020

What it does

ESLint (and its typescript counterpart) have been updated. This really just removes deprecated rules and updates the recommended presets, so not too much here.

How to test

Tests passed locally.

Review checklist

Reminder for reviewers

@akosyakov akosyakov requested review from paul-marechal and kittaakos and removed request for paul-marechal July 8, 2020 07:05
@akosyakov akosyakov added the dependencies pull requests that update a dependency file label Jul 8, 2020
@akosyakov
Copy link
Member

It does not seem that you pushed updated yarn.lock.

Signed-off-by: Reece Dunham <[email protected]>
@RDIL
Copy link
Contributor Author

RDIL commented Jul 8, 2020

Fixed. My Yarn version was newer than Travis's.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I don't really see the benefit of bumping the versions but losing functionality by removing
"@typescript-eslint/class-name-casing": "error". We should find a solution which does not remove one of our rules as it will lead to poorer code quality and issues down the road with naming.

If we are to bump, we should make sure that the rule still works correctly.

For example:

export class a { }

reports an error on master but not this pull-request.

Class 'a' must be PascalCased. eslint(@typescript-eslint/class-name-casing)

If we are to remove the dependency, then we also need to make sure to clean up the rule as it currently complains.

@akosyakov
Copy link
Member

akosyakov commented Jul 8, 2020

There seems to be another rule typescript-eslint/typescript-eslint#2251.

btw I've already upgraded eslint in #7908 I think I've done it to enable latest deprecation rule which was lost during migration from tslint. I would prefer to have deprecation rule working than @typescript-eslint/class-name-casing. I am not even sure we have latter rule with tslint before.

@RDIL
Copy link
Contributor Author

RDIL commented Jul 8, 2020

Alright. I'm just trying to keep everything up-to-date in the toolchain, so I'll look into this or find something else that needs updating.

@marcdumais-work
Copy link
Contributor

Alright. I'm just trying to keep everything up-to-date in the toolchain, so I'll look into this or find something else that needs updating.

Thanks @RDIL

@RDIL RDIL closed this Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants