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 eslint errors #4485

Merged
merged 1 commit into from
Jul 22, 2017
Merged

Fix eslint errors #4485

merged 1 commit into from
Jul 22, 2017

Conversation

benmccann
Copy link
Contributor

Upgrades gulp-eslint and fixes errors

min < 0 && max < 0? max :
min > 0 && max > 0? min :
0;
return me.beginAtZero ? 0 :
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new rule? I don't think it makes the code more readable, I would disable it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so at first too, but then I realized that it more clearly communicates that each line is a continuation of the past and each new line is an else statement of the previous.

In general, I think it's a good rule that continued lines should be indented from the previous. I'm not sure we'd want to disable that entirely because of just one place where we might not like the effect

7, // 5 + 2
5, // 6 - 1
'end', // 'end'
'start', // 'origin'
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to change spaces to tabs for comment alignment?

It breaks when changing editor config, for example (1 tab == 2 spaces):

image

Copy link
Contributor Author

@benmccann benmccann Jul 11, 2017

Choose a reason for hiding this comment

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

I was able to reconfigure the no-multi-spaces option to stop creating errors when it's a comment after a line and reverted the spacing changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crap, this option broke Code Climate because it's only available in eslint v4 and Code Climate only supports eslint v3 :-( codeclimate/codeclimate-eslint#283

@benmccann benmccann force-pushed the gulp-eslint branch 4 times, most recently from 2580be7 to db37286 Compare July 12, 2017 02:27
@benmccann benmccann changed the title Upgrade gulp-eslint Fix eslint errors Jul 12, 2017
@benmccann
Copy link
Contributor Author

I've reverted the actual eslint upgrade for now since codeclimate only supports eslint 3 at the moment. I'd still like to check in all the style fixes though so that when codeclimate adds eslint 4 support in the future it will be an easy upgrade

@@ -170,7 +170,7 @@ module.exports = function(Chart) {
// the canvas render width and height will be casted to integers so make sure that
// the canvas display style uses the same integer values to avoid blurring effect.

// Set to 0 instead of canvas.size because the size defaults to 300x150 if the element is collased
// Set to 0 instead of canvas.size because the size defaults to 300x150 if the element is collased
Copy link
Member

Choose a reason for hiding this comment

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

That's weird ESLint didn't reported these spaces instead of tabs

@simonbrunel simonbrunel merged commit f6b6956 into chartjs:master Jul 22, 2017
@simonbrunel simonbrunel added this to the Version 2.7 milestone Jul 29, 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.

2 participants