Add ESLint support #736

Merged
merged 4 commits into from Aug 12, 2016

Projects

None yet

2 participants

@XhmikosR
Member
XhmikosR commented Aug 7, 2016 edited

First pass, do not merge until ready.

Non-whitespace diff: https://github.com/MaxCDN/bootstrap-cdn/pull/736/files?w=1

/CC @jmervine for thoughts on the rules, scripts etc. Note that I quickly added this in npm scripts, should probably be in make.js.

Fixes #684.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine commented on the diff Aug 7, 2016
scripts/integrity.js
// bootlint
-for (var i = 0; i < config.bootlint.length; i++) {
- var bootlint = config.bootlint[i];
- var file = buildPath(bootlint.javascript);
+(function() {
@jmervine
jmervine Aug 7, 2016 Member

Why the closures? I ask, not to be contrary, but more for my own education. I never know when they're appropriate and when they're not.

@XhmikosR
XhmikosR Aug 7, 2016 Member

I went this way because i and file variables were being redefined in the whole file. So, with IIFE we don't pollute the global scope.

@jmervine
jmervine Aug 7, 2016 Member

Makes sense. Thanks.

@jmervine jmervine commented on the diff Aug 7, 2016
tests/functional_test.js
var expectedHeaders = {
- date: undefined,
- etag: undefined,
- expires: undefined,
+ 'date': undefined,
@jmervine
jmervine Aug 7, 2016 Member

Why quote these?

@XhmikosR
XhmikosR Aug 7, 2016 Member
"quote-props": [2, "consistent"]

Feel free to check out and suggest any other options. I'm not tied to these ones, just tried to be as strict as possible and close to my own personal style.

@jmervine
jmervine Aug 7, 2016 Member

I see, so if some are quoted, it recommends that all in the object are for consistency. That makes sense. I have no strong feelings on it, was just wondering.

@XhmikosR
XhmikosR Aug 7, 2016 Member

http://eslint.org/docs/rules/quote-props

It shouldn't matter in our case that is why I went with consistent.

@jmervine
Member
jmervine commented Aug 7, 2016

This all looks great. My only comment is that I really prefer the tabularized alignment where it makes sense. But I'm willing to forgo it in favor of consistency.

@XhmikosR
Member
XhmikosR commented Aug 7, 2016

I tried to keep that but couldn't find the right config.

Also, there are a few more warnings and errors that we need to sort.

This branch will take some time but should help keep the JS codebase consistent :)

@XhmikosR
Member
XhmikosR commented Aug 7, 2016

OK, I think it's "exceptions": { "VariableDeclarator": true }. I will try it. In the meantime, check out the branch (I won't rebase until we are ready to merge it) and see if you can sort the rest.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@XhmikosR
Member
XhmikosR commented Aug 7, 2016

@jmervine: I'll need to rebase this later and revert the alignment changes. If you have something else to commit here let me kmow.

@jmervine
Member
jmervine commented Aug 7, 2016

Nothing at this time. :D

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 7, 2016 Inactive
@XhmikosR XhmikosR commented on an outdated diff Aug 8, 2016
public/assets/js/main.js
@@ -1,5 +1,6 @@
/* eslint-env browser */
/* exported tryIt, toggleCode */
+'use strict';
@XhmikosR
XhmikosR Aug 8, 2016 Member

This isn't right BTW, I will take care of it tomorrow with stricter options too.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@XhmikosR
Member
XhmikosR commented Aug 8, 2016

@jmervine: I added a few more rules.

About main.js, we need to refactor it to use IIFE and be explicit with global variables. Also, check the new errors when you have some time along with the new rules that I have added and are set to 0.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 8, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-736 Aug 12, 2016 Inactive
jmervine added some commits Aug 7, 2016
@jmervine @XhmikosR jmervine fixing linting issues 55d9c1f
@jmervine @XhmikosR jmervine moving and consolidating lint targets
2ce119d
@XhmikosR
Member

Rebased and pushed. Should be ready for now. We can tweak it later.

@jmervine
Member

I'll merge as soon as CI finishes.

@jmervine jmervine merged commit 7da16e2 into develop Aug 12, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No known vulnerabilities
Details
@jmervine jmervine deleted the xmr-eslint branch Aug 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment