Implement ESLint with eslint-config-edx by bjacobel · Pull Request #75 · openedx/edx-ui-toolkit

bjacobel · 2016-06-20T19:52:31Z

Install ESLint, and adopt our new styleguide.

If you have comments about the ESLint rules that are responsible for suggesting these changes, you can make them here: edx/eslint-config-edx#1. The ruleset/styleguide is not released/finalized yet, but will be 1.0 soon, so hurry! eslint-config-edx is now 1.0 on NPM

Testing Checklist

N/A

Non-testing Checklist

Consider any documentation your change might need, and which users will be affected by this change.

Post-review

Squash commits into discrete sets of changes with descriptive commit messages.
🚨 TODO before merge: update package.json version of eslint-config-edx to NPM 1.0 🚨

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

We'll also automatically suggest reviewers for this PR based on the code it touches.

mention-bot · 2016-06-20T19:52:33Z

By analyzing the blame information on this pull request, we identified @andy-armstrong, @dsjen and @dan-f to be potential reviewers

andy-armstrong · 2016-06-20T19:56:41Z

+    var eslint = require('gulp-eslint'),
        gulp = require('gulp'),
        paths = {
            lint: [


Do we need both this and the .eslintignore? Maybe this can just be **/*.js and let the exceptions be handled in the ignore file.

andy-armstrong · 2016-06-20T20:07:15Z

@bjacobel It looks like you have a leftover call to the old jscs gulp task.

dsjen · 2016-06-21T13:27:08Z

+(function() {
+    'use strict';
+
+    var renameAsMarkdown, generateDocFor;


I guess the convention has been to declare variables in alphabetical order unless declarations get messed up.

I don't see us following this convention anywhere very frequently. Is it something you feel strongly about starting to enforce? We can look into if there's an eslint rule for it.

I wasn't aware of that convention. I tend to declare them in the order I'm going to use them.

I tend to declare them in the order I'm going to use, too.

I'm with @andy-armstrong and @alisan617, I declare them in the order used. Alphabetical could be complicated if var aardvark relies on var zoo.

alisan617 · 2016-06-21T18:26:42Z

    "css-loader": "~0.23.1",
    "del": "~2.2.0",
    "edx-pattern-library": "~0.12.5",
+    "eslint-config-edx": "bjacobel/eslint-config-edx#bjacobel/feedback",


Isn't this supposed to be edx/eslint-config-edx 0.0.1 ?

The 0.0.1 that's on NPM right now was released last Friday so it's a little stale, but I'm going to release a 1.0.0 to NPM after edx/eslint-config-edx#1 merges and then this can be updated to point there.

… - Gulp does this already)

… 1sec

bjacobel · 2016-06-27T18:56:21Z

This has no more linter issues and is ready for review.

~~(There's some issue by which the coverage percentage dropped... I'll look into that.)~~

…n Node

…breaking API)

coveralls · 2016-06-27T19:17:33Z

Coverage decreased (-0.02%) to 94.439% when pulling d774f77 on bjacobel/eslint into 9755c73 on master.

bjacobel · 2016-06-27T19:21:17Z

MRW Coveralls is failing on a 0.02% decrease, but none of the files on Coveralls actually show a decrease in lines covered

bjacobel · 2016-06-27T19:55:22Z

-            }
-        }
+    withData = function(data, func) {
+        Object.keys(data).forEach(function(key) {


Note to reviewers: for (... in ...) is banned by Airbnb ESLint because a) it has performance issues in some browsers and b) it loops over all enumerable properties, not just array members and object keys (e.g., toString).

It also wasn't a particularly good choice here anyway, because data was an Object, not an Array. So I swapped it out for Object.keys.forEach.

PS: In ES6 we can use for (... of ...) to do what for (... in ...) looks like it does.

bjacobel · 2016-06-28T14:17:57Z

this coveralls build: 883 of 935 relevant lines covered
latest master build: 886 of 938 relevant lines covered

So there's three less lines covered, and three less lines overall... according to Coveralls that's a build failure. Wat.

bjacobel · 2016-06-30T16:49:14Z

Hi folks! Would love to get this merged in this week, if we can - that's when I said we'd be turning it on in my email to edx-code.

@dsjen and @andy-armstrong have already done an initial review, mind giving final signoff if I've addressed all comments? @AlasdairSwan and @alisan617, would love to hear your feedback too.

Haven't figured out a good way to address the coveralls failure yet. Wish there was something analogous to [skip ci] commit comments to skip a Travis build. Any ideas?

alisan617 · 2016-06-30T17:10:15Z

LGTM 👍

AlasdairSwan · 2016-06-30T18:38:40Z

@@ -0,0 +1,5 @@
+{
+    "rules": {
+        "no-console": "off"


@bjacobel I quite like this rule, forcing people to merge cleaner code. Is there a reason we would want console.log statements in the codebase? I would be fine keeping warn or error if it is needed but vote to get rid of debugging statements from the code.

ESLint allows per-directory configs, so this only applies to scripts in the gulp folder. I decided to allow console in Gulp scripts because 1) console is always defined in Node [*] and 2) console.log is pretty common in our gulp scripts as a means of feedback.

[*]: one of the arguments in favor of having no-console enabled in our browser code is that console is not implemented in IE9 and below; in Node this shouldn't ever be a problem

Cool, thanks for the explanation.

coveralls · 2016-06-30T18:57:39Z

Coverage decreased (-0.02%) to 94.439% when pulling e9f9f31 on bjacobel/eslint into 9755c73 on master.

AlasdairSwan · 2016-06-30T19:46:31Z

👍

dsjen · 2016-06-30T20:27:17Z

👍

andy-armstrong · 2016-06-30T21:47:22Z

👍

coveralls · 2016-07-01T19:29:37Z

Coverage increased (+0.2%) to 94.67% when pulling 1ffa8e0 on bjacobel/eslint into 9755c73 on master.

andy-armstrong · 2016-07-01T20:45:54Z

@bjacobel Awesome find with the sinon bug! Great coverage improvements too.

andy-armstrong reviewed Jun 20, 2016
View reviewed changes

dsjen reviewed Jun 21, 2016
View reviewed changes

andy-armstrong force-pushed the bjacobel/eslint branch from 224e69b to 01d10aa Compare June 21, 2016 17:26

alisan617 reviewed Jun 21, 2016
View reviewed changes

bjacobel force-pushed the bjacobel/eslint branch from a9c180c to a4ed075 Compare June 24, 2016 17:14

bjacobel added 7 commits June 27, 2016 14:15

Install ESLint with the eslint-config-edx config

fce5599

Do an autofix pass

c3ed0de

Add some files to be ignored by eslint (when running the CLI straight…

cef6541

… - Gulp does this already)

Fix quote-props rule violations

c1475b8

Fix one-var rule violations

26b014b

Fix no-undef rule violations

044b9c6

Fix no-param-reassign rule violations

a62280e

bjacobel force-pushed the bjacobel/eslint branch from a4ed075 to d431511 Compare June 27, 2016 18:42

bjacobel changed the title ~~Implement ESLint with eslint-config-edx (WIP)~~ Implement ESLint with eslint-config-edx Jun 27, 2016

bjacobel and others added 7 commits June 27, 2016 14:50

Fix max-len rule violations

a68d114

Remove jshint directives

037b6af

Do another autofix pass to fix space-before-function-paren

9ee692a

Remove call to removed JSCS task

280f988

Piping a file glob thru gulp-eslint takes 20sec? with gulp-shell it's…

1b644ba

… 1sec

Fix common errors in all gulp scripts: strict, wrap-iife, global-require

2bb0fe8

Adjust dependencies

bc30575

bjacobel force-pushed the bjacobel/eslint branch from d431511 to 9a20fa8 Compare June 27, 2016 18:55

Address ESLint failures

652d28a

bjacobel added 9 commits June 27, 2016 15:11

Do another autofix pass after rebasing and upgrading to 1.0.0

e34e5a3

Allow console.log in the gulp dir because console is always defined i…

c84f408

…n Node

Fix global-require rule violations

55a9429

Fix no-unused-vars rule violations

feea45c

Fix strict rule violations

05f52ed

Fix no-restricted-syntax rule violations

3d06e8a

Fix (ignore) no-underscore-dangle rule violations

0639537

Fix wrap-iife rule violations

08051ec

Fix (ignore) no-param-reassign rule violations (Fixing would require …

08c38ab

…breaking API)

bjacobel force-pushed the bjacobel/eslint branch from 9a20fa8 to d774f77 Compare June 27, 2016 19:11

bjacobel reviewed Jun 27, 2016
View reviewed changes

AlasdairSwan reviewed Jun 30, 2016
View reviewed changes

Code review fixes

e9f9f31

bjacobel force-pushed the bjacobel/eslint branch from d774f77 to e9f9f31 Compare June 30, 2016 18:52

Sinon 1.17.4 has a nasty bug, these tests work fine on 1.17.3

1ffa8e0

bjacobel merged commit d0378bb into master Jul 1, 2016

bjacobel deleted the bjacobel/eslint branch July 1, 2016 19:31

Conversation

bjacobel commented Jun 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing Checklist

Non-testing Checklist

Post-review

Reviewers

Uh oh!

mention-bot commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-armstrong commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjacobel commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjacobel commented Jun 27, 2016

Uh oh!

bjacobel Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjacobel commented Jun 28, 2016

Uh oh!

bjacobel commented Jun 30, 2016

Uh oh!

alisan617 commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjacobel Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlasdairSwan commented Jun 30, 2016

Uh oh!

dsjen commented Jun 30, 2016

Uh oh!

andy-armstrong commented Jun 30, 2016

Uh oh!

coveralls commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-armstrong commented Jul 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bjacobel commented Jun 20, 2016 •

edited

Loading

bjacobel commented Jun 27, 2016 •

edited

Loading

coveralls commented Jun 27, 2016 •

edited

Loading

bjacobel Jun 27, 2016 •

edited

Loading

bjacobel Jun 30, 2016 •

edited

Loading

coveralls commented Jun 30, 2016 •

edited

Loading

coveralls commented Jul 1, 2016 •

edited

Loading