Migrate to ESLint

[Silverwolf90]

As we start to move over to ES6, I wanted to propose using ESLint over JSHint.

Reasons:

• More flexible, powerful
  • Larger set of more configurable rules
  • Supports custom rules, we could create our own vexflow specific set of eslint rules that suggest best practices, or warn of improper usage.
• Some errors can be automatically fixed with a CLI flag: eslint --fix
• It's currently winning the linter wars
  • JSCS team decided to make 3.0 the last major version and join the ESLint team

I personally use the airbnb style guide's eslint-config, with some tweaks. For the most part it's a great starting place, but I find it overly strict in some cases. But in general, VexFlow would benefit significantly from having stricter linting rules.

[Silverwolf90]

My ideal .eslintrc.json would look something like this:

{
  "extends": "airbnb",
  "rules": {
    "func-names": [0],
    "max-len": [2, 80],
    "new-cap": [0],
    "no-case-declarations": [2],
    "no-confusing-arrow": [0],
    "no-else-return": [0],
    "no-multi-spaces": [0],
    "no-param-reassign": [0],
    "no-shadow": [0],
    "no-use-before-define": [0],
    "prefer-template": [0],
    "space-before-function-paren": [2, "never"],
    "strict": [2, "global"]
  }
}

[aaronmars]

Yes! I support this 100%.

[mscuthbert]

I like this a lot too (and thanks for the great link to the airbnb code). One thing that the new class system proposal would run afoul of is the export class Thing ... paradigm -- I think I like the airbnb idea of imports at the top of the file and exports at the bottom of the file.

[Silverwolf90]

WIth the above config:

Here's the result of running eslint --fix

Here's annotation.js after manually editing remaining lint errors

[0xfe]

Looks great. I like the final annotation.js too.

I don't know what all the options mean, but I'm happy to look through them in detail. (E.g., I used to care about 80-char line lengths, but it's a bit of an uphill battle, and I'm not sure if there's any value in it anymore.)

[Silverwolf90]

Initial PR with four files here: #369

Wiki page which outline basic steps to convert a file

[Silverwolf90]

I think it's probably worth doing all the automatic steps at once. Here's a branch (continuing off of the previous PR) which shows the codebase after running every file through lebab w/ only safe transforms, followed by eslint --fix.

After the automatic steps, we're at 892 problems. Then I disabled the quote-props rule and the problems lower to 540. Compared to the initial 8k problems, this is starting to seem reasonable!

[mscuthbert]

540 problems sounds great to deal with! Should we mark pull requests to go into that branch instead for now?

I'd also be in favor of dropping 80 characters or at least moving to something more like 100 or 130. I prefer not to split quoted strings/error messages, even if it goes over the limit, so that they can be found in a search on the error message. (I.e., L('Massive Error in the ' +
'beaming routine') which won't be found in a search for "Massive Error in the beaming routine");

[Silverwolf90]

@mscuthbert Agreed. My earlier comment about the config is out of date. Following @0xfe's comment I bumped it up to 100. And I disabled a couple not-so-important rules to make the migration easier.

Also, those split strings are usually great candidates for conversion to ES6 template strings.

On the branch I just posted, the config file looks like this:

{
  "extends": "airbnb-base",
  "root": true,
  "rules": {
    "max-len": [2, 100],
    "camelcase": [0],
    "no-case-declarations": [2],
    "no-confusing-arrow": [0],
    "new-cap": [0],
    "no-else-return": [0],
    "no-multi-spaces": [0],
    "no-param-reassign": [0],
    "no-shadow": [0],
    "no-use-before-define": [0],
    "quote-props": [0],
    "prefer-template": [0],
    "space-before-function-paren": [2, "never"],
    "strict": [2, "global"]
  }
}

I'm happy to accept PRs to that branch. @0xfe do you have a preference on how we go about this?

[Silverwolf90]

Also, worth noting that I've been reformatting errors to look like this:

// If it fits on one line...

if (!this.accidental) {
  throw new Vex.RERR('ArgumentError', `Unknown accidental type: ${type}`);
}

// If it's a little too long...

if (!this.accidental) {
  throw new Vex.RERR(
    'ArgumentError', `Unknown accidental type: ${type}. Extra stuff here to make the line longer.`
  );
}

// if it's way too long

if (!this.accidental) {
  throw new Vex.RERR(
    'ArgumentError',
    `Unknown accidental type: ${type}. Extra stuff here to make the line longer. And even longer!`
  );
}

[Silverwolf90]

Updated the wiki, added a todo list of all files, and checked off those which are complete.

[mscuthbert]

Super basic question, but how to we get the list of errors for each file? Simply run grunt? or...

[Silverwolf90]

Ah, of course I forgot the basic bits. You have two options:

Running grunt eslint will run eslint on all the ESLINT_SOURCES expected to pass in the Gruntfile. You could just add a failing file to that array.

Or you can run eslint independently of grunt:

cd <vexflow root>
npm install
eslint <path>

[0xfe]

Sending PRs to Silverwolf's branch sounds good. Cyril can you send me PRs every time you have a few files ready. It's easier to review in smaller batches (and also to debug if things go wrong.)

[Silverwolf90]

Yep that sounds good!

[Silverwolf90]

Although I would suggest we do all the safe automatic transforms at once across the codebase in one PR. And from there go a few files at a time.

[aaronmars]

EDIT: Ok. Looks like we can do it with master then!

So, am I reading this right that I can do this?

• Claim a few files
• Checkout from Silverwolf's eslint-all branch
• Fix eslint errors
• Send a PR to the branch above

[0xfe]

Yes! 👍

I guess this issue is the best place to claim your files. First come first serve. :-)

[Silverwolf90]

Just FYI that I've completed these already:

accidental.js
clef.js
formatter.js
frethandfinger.js
modifiercontext.js
stave.js
staveconnector.js
stavenote.js
stringnumber.js
tremolo.js
tuning.js
vex.js
vibrato.js
voice.js

[aaronmars]

I'll have a go at the following (if this causes too much insanity with merges, please let me know):

annotation.js
articulation.js
barnote.js
beam.js
bend.js

EDIT: Done in #375

[Silverwolf90]

Note that in #374 I've removed the restriction on the ForInStatement as it appears in VexFlow quite a bit.

[Silverwolf90]

Just completed strokes.js

Edit: Only 360 errors remain!

[Silverwolf90]

Just opened a PR where I've just taken care of:

frethandfinger.js
music.js
stemmablenote.js
tables.js

EDITS:
I've also completed:
boundingbox.js
boundingboxcomputation.js
canavscontext.js
crescendo.js
dot.js
fraction.js
glyph.js
gracenote.js
gracenotegroup.js
keymanager.js
keysigntuare.js
modifier.js
note.js
notehead.js
ornament.js
pedalmarking.js
raphaelcontext.js
renderer.js
stavebarline.js
stavehairpin.js
staveline.js
staverepetition.js
stavetempo.js
stavetext.js
stavetie.js
stavevolta.js
stem.js
svgcontext.js
tabnote.js
tabslide.js
tabtie.js
textbracket.js
textdynamics.js
textnote.js
tickable.js
tickcontext.js
timesignature.js
tuplet.js
0 problems left!

[Silverwolf90]

Finished!

EDIT: Branch here https://github.com/Silverwolf90/vexflow/tree/eslint-complete

[0xfe]

\o/ You're a superstar, Cyril!

Just merged everything! You deserve the honors of closing this issue. :-p

[Silverwolf90]

Awesome! And thank you! Closing.

[mscuthbert]

Whooooooo! Hoooooooo! :-)

[gristow]

Wow -- bravo!