Skip to content

Upgrade to eslint 4 - #4738

Merged
simonbrunel merged 1 commit into
chartjs:masterfrom
benmccann:eslint4
Nov 1, 2017
Merged

simonbrunel merged 1 commit into
chartjs:masterfrom
benmccann:eslint4

Conversation

@benmccann

Copy link
Copy Markdown
Contributor

I had to add one extra set of parentheses. I filed as an eslint bug for that one: eslint/eslint#9285

Otherwise I fixed all the issues in past PRs

@simonbrunel

simonbrunel commented Sep 11, 2017

Copy link
Copy Markdown
Member

Do we really want to upgrade to ESLint 4 if it's bugged and forces us a syntax we don't want? what are the benefits?

@benmccann

benmccann commented Sep 12, 2017

Copy link
Copy Markdown
Contributor Author

ESLint 4 found a good number of real issues as well. I fixed all those in a separate commit earlier: f6b6956

It's only this one place in the code base that there was a regression in behavior. I think all the real issues it found may outweigh having to add parens in one spot

@benmccann
benmccann force-pushed the eslint4 branch 2 times, most recently from b34b2aa to 7c9d0cc Compare September 17, 2017 16:59
@benmccann

benmccann commented Sep 17, 2017

Copy link
Copy Markdown
Contributor Author

I've updated this to use the newest eslint with a fix for the bug I reported. It now passes locally without needing the extra set of parens

It's still failing on codeclimate. It must not be using the latest version of eslint. I've sent a PR to upgrade codeclimate: qltysh-archive/codeclimate-eslint#326

@benmccann
benmccann force-pushed the eslint4 branch 2 times, most recently from 57369a6 to 633f099 Compare September 19, 2017 21:08
@benmccann

benmccann commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

@simonbrunel I now have eslint 4 working both locally and on codeclimate without any code formatting changes. You should be able to merge this PR now. Thanks for the review and feedback

@etimberg etimberg added this to the Version 2.8 milestone Sep 29, 2017
@benmccann

Copy link
Copy Markdown
Contributor Author

@etimberg @simonbrunel would we be able to merge this now? Even if we want to have a 2.7.2 I think this PR should still be okay

@etimberg

etimberg commented Nov 1, 2017

Copy link
Copy Markdown
Member

I am ok with this

@simonbrunel
simonbrunel merged commit ffbdb48 into chartjs:master Nov 1, 2017
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants