ESLint Updates #3308
ESLint Updates #3308
Conversation
| @@ -405,7 +408,7 @@ module.exports = function(Chart) { | |||
| } | |||
| } | |||
| } | |||
| }).call(me); | |||
| }.call(me); | |||
There was a problem hiding this comment.
can probably remove the .call(me) because me is captured by scope now. that could be a different PR though
| @@ -442,7 +445,7 @@ module.exports = function(Chart) { | |||
| } | |||
| } | |||
| } | |||
| }).call(me); | |||
| }.call(me); | |||
There was a problem hiding this comment.
same thing here about .call(me)
| @@ -31,20 +31,20 @@ module.exports = function(Chart) { | |||
|
|
|||
| // Helper function to draw a line to a point | |||
| function lineToPoint(previousPoint, point) { | |||
| var vm = point._view; | |||
| var pointvm = point._view; | |||
There was a problem hiding this comment.
minor nitpick, I think this is better as pointVM.
|
@etimberg Updated to include your suggestions |
|
awesome! LGTM! |
| @@ -405,7 +408,7 @@ module.exports = function(Chart) { | |||
| } | |||
| } | |||
| } | |||
| }).call(me); | |||
| }; | |||
There was a problem hiding this comment.
probably need to call function and set return to found. maybe .call is right 😕
this caused the test fails
|
@etimberg Changed to only include |
|
thanks :) |
| no-cond-assign: 2 | ||
| no-console: 0 | ||
| no-console: 1 |
There was a problem hiding this comment.
Could be set as "error" as you had in your Gist, but maybe only for console.log (so no-console: [2, { allow: [warn, error] }])?
| @@ -96,7 +96,7 @@ rules: | |||
| radix: 2 | |||
| vars-on-top: 0 | |||
| wrap-iife: 2 | |||
| yoda: 0 | |||
| yoda: [1, never] | |||
There was a problem hiding this comment.
This one was "error" in your Gist, is there cases where we can't adapt the code?
There was a problem hiding this comment.
When there is a comparison Val < raw number < otherval it gives an error but that is really the best way to have that statement
There was a problem hiding this comment.
Ok, I don't really mind about that rule, was just curious why :)
| no-unused-vars: 0 | ||
| no-use-before-define: 0 | ||
| no-unused-vars: 2 | ||
| no-use-before-define: 1 |
There was a problem hiding this comment.
I was having problems with that in a certain file but I'm going to try to go back and see if I can fix it so the rule can be set as error
| @@ -77,7 +77,7 @@ module.exports = function() { | |||
| onClick: null, | |||
| defaultColor: 'rgba(0,0,0,0.1)', | |||
| defaultFontColor: '#666', | |||
| defaultFontFamily: "'Helvetica Neue', 'Helvetica', 'Arial', sans-serif", | |||
| defaultFontFamily: '\'Helvetica Neue\', \'Helvetica\', \'Arial\', sans-serif', | |||
There was a problem hiding this comment.
Very minor, but I think I prefer the previous version which is easier to read and less character. What do you think? should we only allow double quotes to avoid escaping (quotes: [2, single, { avoidEscape: true }])?
There was a problem hiding this comment.
I agree, this change was automatically done with --fix so I didn't realize what it looked like
|
A few minor comments, that's a great work @zachpanz88 |
| func-names: 0 | ||
| brace-style: [2, 1tbs] | ||
| camelcase: 2 | ||
| comma-dangle: [2, never] |
There was a problem hiding this comment.
Just remembered about that comment and I agree with rjmcguire. Should we allow (but not enforce) trailing comma in this special case (comma-dangle: [2, only-multiline])?
|
@simonbrunel updated to include your changes |
|
Clear to merge? |
|
Just pulled these changes locally and didn't think about the AutoCrlf git option which conflict with |
|
Yep, you can go ahead and commit that |
|
Fine to change |
These updates add more structure to code styling. Tests will now fail if the code does not match the style set forth by these guidelines. The official standards were created based off of the most commonly used styles in this project already.
Hopefully this will make code easier to read over time and will keep all styling uniform throughout the project.
Eventually these rules should be extended to all code in the project (tests, etc.) rather than just the src files.
I did my best to make sure that no functionality was changed through the adaptation of this styling throughout the project, but I might have caused a bug somewhere so please make sure that my changes will not induce any unexpected behavior.