Add a shared ESLint config based on Airbnb's ES5 styleguide, document our customizations by bjacobel · Pull Request #1 · edx/eslint-config-edx

bjacobel · 2016-06-16T21:23:29Z

Looking for feedback on both the linting rules this would have us adopt in our Javascript, and my efforts at documenting them.

TODO: update version to 1.0, release to NPM
TODO: move this to an edx repo

bjacobel · 2016-06-16T21:27:09Z

Note to reviewers: following this link allows you to see the README.md as rendered markdown, which is a lot more readable

bjacobel · 2016-06-16T21:45:39Z

Also CCing @dan-f, I know you were looking forward to seeing this effort pushed forward.

dan-f · 2016-06-17T01:06:53Z

cool, ty @bjacobel!

andy-armstrong · 2016-06-17T06:12:14Z

+    "jquery": true,
+    "amd": true
+  },
+  "extends": "airbnb-base/legacy",


For reviewers: this comes from here:

https://www.npmjs.com/package/eslint-config-airbnb-base

put this in an alphabetically order? so and comes first

Good catch.

andy-armstrong · 2016-06-17T06:34:58Z

Fantastic stuff @bjacobel. It will be very interesting to see your UI Toolkit PR and see how these rules play out in practice. I'd also like to hear from @pdesjardins about where he sees the various pieces of the documentation living.

You should also talk to IT and/or devops about creating a new repo in the edx organization. When you create an npm package for this, can you also add all the usual suspects as collaborators.

bjacobel · 2016-06-17T15:39:21Z

@andy-armstrong Cool, I'll work on getting this into the edx github org and doing an npm release after this merges to master.

andy-armstrong · 2016-06-17T15:45:16Z

+  "description": "ESLint config for edX JavaScript code.",
+  "main": "index.js",
+  "scripts": {
+    "test": "eslint ."


I was thinking about this line, and how we could test the rules. One idea I had is to include a 'samples' directory with code demonstrating each of the rules we care about. Then we could have CI actually run eslint and fail if any of the samples have errors. What do you think of that?

It would also be good to have some negative tests, but I don't know how you could achieve that. Maybe you could have a separate directory for code with issues, and then run eslint twice. The bad examples would have an expected number of eslint failures.

Are there any good examples of how to write tests for eslint rules?

Not 100% sure I understand you here - you're saying we should test our lint ruleset by providing samples of code that should fail, and testing that they do? ESLint already does that when they ship, though.

I guess beyond the correctness of ESLint rules in actually doing what they say they do, we could also get into a situation where we create a contradiction (eg, object properties have to be in kebab case, but you can't quote them). I don't know if unit testing helps us catch that though, just running it on our real codebase should be enough.

My reason for putting a test line here at all was mostly as a joke/easter egg... I thought it was funny that our lint repo could pass its own linter :)

Sorry that wasn't clear. I was thinking we could provide good representative code samples that would pass the linter. The documentation then could point people at that code to say here's how we expect our code to look. These samples would demonstrate the rules that we have changed from AirBnB.

I was then wondering about negative tests but that seems less important, and is a little complex to make work.

I actually really like that the lint repo passes it's own linter! It would probably prevent errors from creeping in too. For a while I had a linter running on my gulp config file and it caught some errors early on!

…tent") Fixing all the camelcase violations with properties: always on would have required breaking the UITK's API. So turn properties: to never, which is Airbnb default, so remove the rule. Once you open up the possibility of snake/kebab-case object properties, quoting becomes an issue again. I don't like the Airbnb config's rule here, it leads to having to quote some things and not other things. So use this rule with "consistent" setting, which I've documented in the README.

bjacobel · 2016-06-17T18:17:56Z

I altered the behavior of both the quote-props and camelcase rules, see the commit message of 0b41eed for more explanation.

bjacobel · 2016-06-20T14:31:36Z

There's been some discussion of the space-before-function-paren rule, which is currently set to { anonymous: 'always', named: 'never' } by Airbnb default.

That means the following patterns are correct:

function foo() {

}

var bar = function () {

};

And the following patterns will be a linter error:

function foo () {

}

var bar = function() {

};

My opinions: named: 'never' is good because it reinforces the same spacing as you'd use when calling the function (e.g., foo(1)). I don't have a strong opinion on whether or not the anonymous functions should or shouldn't have the space, but I am for keeping our overrides of the airbnb style as few as possible. If we were to override the airbnb rule, I would support 'never' in both for consistency.

cc @andy-armstrong @cahrens

andy-armstrong · 2016-06-20T14:46:12Z

@bjacobel Thanks for the explanation. My preference is to support never for both cases, both for consistency and because that's how edX has always done things. However, I can see the benefit to minimizing the number of places we override the AirBnB rules, so I'm willing to be overruled if others disagree.

@cahrens @AlasdairSwan @dsjen @atang, what say you?

cahrens · 2016-06-20T14:47:44Z

I agree-- "never" in both cases. Why make them different?

bjacobel · 2016-06-20T14:53:46Z

Here's the airbnb rationale for why they set it up that way (it's only in the ES6 guide, not the ES5 one, which is why I couldn't find it at first): https://github.com/airbnb/javascript#functions--signature-spacing

(here's the PR that introduced it: airbnb/javascript#605)

I think in their mind { anonymous: 'always', named: 'never' } is the consistent approach? But I don't see the case of "I have an anonymous function and I want to give it a name so now I have to change the spacing" to be very common.

alisan617 · 2016-06-20T15:01:02Z

I agree both have --never. It has to be consistent and easy to remember.

bjacobel · 2016-06-20T18:03:03Z

Added new commit with:

"space-before-function-paren": ["error", "never"],

FYI, several people have mentioned our JS package naming convention of edx-*, which I looked into changing this to use. ESLint is a bit picky about the names it will allow you to use if you are using a "shareable config" - see this PR. The only two formats that work right now are eslint-config-[name] and @[name-of-npm-user]/eslint-config. So there's no way to start the npm package with just edx-. We can still change the name of the Github repo to be edx-config-airbnb if there's people in support of that, but I think the NPM package name has to stay the same.

andy-armstrong · 2016-06-20T20:20:35Z

@bjacobel This looks great conceptually, and the changes necessary in your UI Toolkit PR all seem reasonable. Excellent stuff! 👍

bjacobel · 2016-06-20T20:36:52Z

I added a checklist for reviewers up top, I'd like to get wide agreement from people before closing this feedback PR as this is something you'll all have to deal with when writing JS :) If others besides Andy have no more feedback would they mind checking off their boxes (and adding anyone else they think should see this)?

bjacobel · 2016-06-20T20:37:42Z

@andy-armstrong do you think we should move the main section of the README into the developers guide before closing this?

andy-armstrong · 2016-06-20T20:45:39Z

@bjacobel Let's hold off on any developer's guide changes until @pdesjardins is available to review it. I think merging it here makes sense for this PR.

alisan617 · 2016-06-20T21:08:13Z

👍 +1

dsjen · 2016-06-21T12:51:59Z

@bjacobel -- thank you for putting this together! I'm going to look at this today.

dsjen · 2016-06-21T13:05:33Z

+        }
+
+        var foo2 = {
+            'bar': 'buzz',


Wouldn't 'bar': 'buzz' should throw an error?

Because the style is set to consistent, quoting is all-or-nothing: either none of them get it, or they all do. Often they all will because one property is not an allowed object identifier, which is what's going on here.

(Although your comment lead me to realize the docs here are actually wrong, pattern I've documented would pass consistent-as-needed, not consistent... hmmmmm)

I'm going to change our rule to consistent-as-needed because that's what I actually documented here mistakenly so I'll assume that that's what the people who already thumbsedup wanted when they reviewed.

dsjen · 2016-06-21T21:43:33Z

👍 🎆 💃

pdesjardins · 2016-06-21T22:15:22Z

+
+## License
+
+The code in this repository is licensed the Apache 2.0 license unless otherwise


The code in this repository is released under the Apache 2.0 license?

Yeah, thoughts on that? I just added that because the Toolkit is Apache and has the same line in its README. Technically there's only one line of code in this repo, is Apache the right choice for that situation? The Airbnb styleguide / linter config this extends is MIT-licensed.

Oh, I just realized you're commenting on the fact that this line is grammatically incorrect, not specifically on the Apache part of it.

I checked with the OSS team and some other members of Docs and verified that Apache is the right license for this project.

pdesjardins · 2016-06-21T22:25:24Z

Looks good, I support merging as is, although I added a few comments. I'd like to capture some of the information in the developer's guide, as described by Andy. 👍

AlasdairSwan · 2016-06-22T17:47:31Z

Looks good @bjacobel 👍

AlasdairSwan · 2016-06-22T18:43:05Z

@bjacobel why did you turn off the 1 var rule? I like it for hammering home the hoisting issue for developers not as familiar with JavaScript. I also worry that the new rule is a little vague (use 1 var if you want, or multiple only specific cases).

bjacobel · 2016-06-22T18:55:12Z

@AlasdairSwan Eek, yeah, I probably should have socialized that change a little more.

Couple of reasons why:

Our JSCS config actually allowed it before, so we're being consistent with the previous style (even though quite a lot of our code does use just one var)
As we move towards writing ES2015 we won't need to worry about the issues that made one var a good idea (because let and const are block scoped) so we can start to ease back on enforcing knowledge of hoisting IMO
Christina was opposed to it and I thought she was a good barometer for how people who aren't 100% JS devs would feel about that rule

IMO, having knowledge that strange things can happen if you don't use just one var is good to have and should make it into our developer's guide, but it's not a common enough concern to mandate that style everywhere.

Thoughts?

bjacobel · 2016-06-22T19:04:44Z

I'm going to make a separate issue for discussing this because I just figured out some new information

AlasdairSwan · 2016-06-22T19:07:26Z

I would prefer to discuss it in more detail before making that change. I think that the move to ES2015 is going to take a while and what I like about enforcing the rule is that it highlights how browsers actually process the JS. I would prefer that the whole rule is omitted in this PR and a separate PR is opened to discuss our agreed upon approach.

bjacobel added 4 commits June 16, 2016 17:17

Initial commit

36df60d

Wrote some documentation, still need to document specific rule overrides

548e929

Package can lint itself with 'npm test'

4c4bac0

Finish out documentation

b90c18b

Standardize on 'correct pattern' / 'linter error'

5847d18

andy-armstrong reviewed Jun 17, 2016
View reviewed changes

Fix indentation in docs

5fbea8b

andy-armstrong reviewed Jun 17, 2016
View reviewed changes

bjacobel force-pushed the bjacobel/feedback branch from 9f05b1e to 0b41eed Compare June 17, 2016 18:17

bjacobel mentioned this pull request Jun 20, 2016

Convert XBlock runtime code from CoffeeScript openedx/openedx-platform#12780

Merged

6 tasks

Add and document space-before-function-paren rule (set to 'never')

5c5d6fa

bjacobel mentioned this pull request Jun 20, 2016

Implement ESLint with eslint-config-edx openedx/edx-ui-toolkit#75

Merged

7 tasks

Alphabetize envs

fc635e8

dsjen reviewed Jun 21, 2016
View reviewed changes

bjacobel added 3 commits June 21, 2016 12:48

Update quote-props rule to match docs (consistent-as-needed)

6c8b2ba

Update repo links to point to new home (edx/eslint-config-edx)

2a55ee3

Set deps up how eslint-config-airbnb-base does (support npm 2 and 3)

f06996b

bjacobel force-pushed the bjacobel/feedback branch from c6718bc to f06996b Compare June 21, 2016 17:52

bjacobel added 2 commits June 21, 2016 16:00

Revert to Airbnb default for 'no-invalid-this' ('off')

db61d8f

Relax one-var rule to a warning

4997cbd

pdesjardins reviewed Jun 21, 2016
View reviewed changes

Tighten up docs

4cd1091

bjacobel added 2 commits June 22, 2016 14:39

Turn off one-var rule entirely

51a7ed4

Clarify docs for this rule

6ff7718

bjacobel merged commit 5ee6c95 into master Jun 22, 2016

bjacobel deleted the bjacobel/feedback branch June 22, 2016 18:41


		## License

		The code in this repository is licensed the Apache 2.0 license unless otherwise

Conversation

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

Uh oh!

Uh oh!

bjacobel commented Jun 16, 2016

Uh oh!

bjacobel commented Jun 16, 2016

Uh oh!

dan-f commented Jun 17, 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!

andy-armstrong commented Jun 17, 2016

Uh oh!

bjacobel commented Jun 17, 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!

bjacobel commented Jun 17, 2016

Uh oh!

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

Uh oh!

Uh oh!

andy-armstrong commented Jun 20, 2016

Uh oh!

cahrens commented Jun 20, 2016

Uh oh!

bjacobel commented Jun 20, 2016

Uh oh!

alisan617 commented Jun 20, 2016

Uh oh!

bjacobel commented Jun 20, 2016

Uh oh!

andy-armstrong commented Jun 20, 2016

Uh oh!

bjacobel commented Jun 20, 2016

Uh oh!

bjacobel commented Jun 20, 2016

Uh oh!

andy-armstrong commented Jun 20, 2016

Uh oh!

alisan617 commented Jun 20, 2016

Uh oh!

dsjen commented Jun 21, 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!

dsjen commented Jun 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjacobel Jun 22, 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!

Choose a reason for hiding this comment

Uh oh!

pdesjardins commented Jun 21, 2016

Uh oh!

AlasdairSwan commented Jun 22, 2016

Uh oh!

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

bjacobel commented Jun 16, 2016 •

edited

Loading

bjacobel commented Jun 20, 2016 •

edited

Loading

bjacobel Jun 22, 2016 •

edited

Loading

AlasdairSwan commented Jun 22, 2016 •

edited

Loading