ESLint

[RunDevelopment]

This PR fixes the indentation errors of all language definitions.
For the vast majority of files, this doesn't do much. A space or two and an additional line at the end of the file. (I used VS Code's auto formatter.)

This also enforces the widespread convention that the names of tokens are surrounded by single quotes.

---

Edit:

This PR now introduces ESLint to the Prism code base with the following features:

• All JS files are checked except for minified or generated files and external libraries.
• ESLint rules are defined in the .eslintrc.json. This includes both stylistic rules as well as some best practices and possible errors.
• gulp integration:
  • The entire code base can be checked using npx gulp lint.
  • The lint task is also part of the default task. (This also ensures that Travis CI uses the linter.)
  • Fixable errors can be fixed using npx gulp lint-fix.

2 new dev dependencies were added (gulp-eslint, gulp-if).

[mAAdhaTTah]

@LeaVerou @Golmote Would an ESLint config be worth introducing?

[LeaVerou]

Yes, that would be great!
You can probably copy the one from Mavo as a starting point: https://github.com/mavoweb/mavo/blob/master/.eslintrc.json

[RunDevelopment]

Hmmm. It seems like gulp-eslint is incompatible with NodeJS v4 because it uses the spread operator.

Any idea on how to fix this?

[mAAdhaTTah]

If eslint itself is compatible with Node 4, we could just put it in pkg#scriptsand have travis run that, and even put the gulp test command there. Otherwise, I don't personally have a problem with dropping Node 4 from our test matrix, considering its EOL already.

[RunDevelopment]

ESLint dropped supporting Node.js 4 in April.

[mAAdhaTTah]

Marked as "need consensus." Definitely need @LeaVerou & @Golmote to weigh in on this ruleset.

[RunDevelopment]

Definitely, yes.
I simply looked through the list of ESLint rules picked out what I thought was useful.

Also, there is the issue that (because I used the VSCode auto formatter) all pairs of 4 spaces are converted into one tab, breaking the tabs for indent and spaces for alignment rule.
I will have to manually reintroduce that before we (hopefully) merge.

[LeaVerou]

Hey @RunDevelopment, thanks for submitting this!

Definitely need @LeaVerou & @Golmote to weigh in on this ruleset.

Yes, this needs serious review by all of us.

I see a ton of changes (229 files changed?!). I would prefer to pave the cowpaths and pick rules that are actually mostly enforced already, than to make such extensive changes to the codebase.

I also disagree with several of these rules. Many seem to be applying an arbitrary code style that neither matches the project, nor has any objective reasoning (e.g. enforce quotes in object keys?! Why?!). Also, our indentation is not tabs, it's smart tabs, i.e. tabs for indentation and spaces for alignment.

I'm also opposed to overlinting, as it tends to get in the way. Let's start with a small set that we mostly follow already, and expand as needed. I suggested what I consider a good starting point above, but did not see a response. Is there a reason you feel we need more rules? Do you disagree with any of the rules there?

Comments would also be useful for reviewing this, both now and in the future, so that people don't have to look up every single cryptic ESLint code to understand what's going on.

I also disagree with using errors for this. Anything related to linting should just be a warning. An error is something that would actually break code, not code style!

[RunDevelopment]

@LeaVerou

229 files changed?!

Well, almost every language definition, plugin and most of the minified files thanks to "no-useless-escape".

I would prefer to pave the cowpaths and pick rules that are actually mostly enforced already, than to make such extensive changes to the codebase.

I don't think that that is possible. Many language definitions have vastly different from e.g. core and javascript (which I consider exemplary files) when it comes to spacing.
Even your suggest rules caused like 1k warnings and a few hundred errors if I remember correctly.
The point is that most files are going to change if we introduce ESLint, so why not consider this a stylistic spring cleaning. The rules are still up in the air and I will gladly implement the style we decide upon.

e.g. enforce quotes in object keys?! Why?!

Do you mean "the names of tokens are surrounded by single quotes"?
The rationale behind this is to clearly distinguish between token names (e.g. class-name, number) and Prism properties (e.g. pattern, rest). This makes language definitions way easier to read imo.
To be clear: This only applies to language definitions.

Also, our indentation is not tabs, it's smart tabs, i.e. tabs for indentation and spaces for alignment.

Well, it still is "no-mixed-spaces-and-tabs": [ 2, "smart-tabs" ]. ("indent" cannot have the value "smart-tabs")
But you are right that right now, we don't use smart-tabs because I used to VS Code auto formatter to fix the spacing of most files and as mentioned before, converts 4 spaces into one tab. Bye bye spaces for alignment.
I actually plan to reintroduce the spaces before we (hopefully) merge but I want to apply that rule to all files consistently and not just to core. (And even core ignores that rules in many places.)
But because there is no nice fixable ESLint rule for that (at least nothing that I know of), it is going to be tedious manual work, so I want to do that right be merging so that there isn't even the possibility that I will have to do it again.

I suggested what I consider a good starting point above, but did not see a response. [...] Do you disagree with any of the rules there?

I actually used your suggestion as the starting point and went from there. Some rules I had to change ("quotes": [1, "double", "avoid-escape"]) and others I added.
But since you don't even recognize it anymore I suppose I overdid it a little...

Is there a reason you feel we need more rules?

I simply consider many rules of ESLint to be very useful and think we should encourage contributors to follow best practices and avoid possible errors (by force).

Comments would also be useful for reviewing this [...]

I honestly did not know that this was possible with ESLint because JSON does not allow comment by default.
I will add them when I have time.

I also disagree with using errors for this.

I made this decision with Travis CI in mind because if a build fails, the contributor will see that change is required without anyone having to tell them. This is supposed to speed up the review process as the linter will point out common mistakes like wrong indentation.

[LeaVerou]

Do you mean "the names of tokens are surrounded by single quotes"?
The rationale behind this is to clearly distinguish between token names (e.g. class-name, number) and Prism properties (e.g. pattern, rest). This makes language definitions way easier to read imo.
To be clear: This only applies to language definitions.

Ok, agreed.

I actually plan to reintroduce the spaces before we (hopefully) merge but I want to apply that rule to all files consistently and not just to core. (And even core ignores that rules in many places.)
But because there is no nice fixable ESLint rule for that (at least nothing that I know of), it is going to be tedious manual work, so I want to do that right be merging so that there isn't even the possibility that I will have to do it again.

Ok. Thank you for being willing to do the tedious manual work.

I simply consider many rules of ESLint to be very useful and think we should encourage contributors to follow best practices and avoid possible errors (by force).

I disagree with blanket adoption of "best practices" lists. We should evaluate each of these practices and see if we want it in our project.

I honestly did not know that this was possible with ESLint because JSON does not allow comment by default.
I will add them when I have time.

Thank you. I will wait until then to review rules in more detail.

I made this decision with Travis CI in mind because if a build fails, the contributor will see that change is required without anyone having to tell them. This is supposed to speed up the review process as the linter will point out common mistakes like wrong indentation.

I'm generally opposed to linting by force. Linting is helpful to point out potential issues, but ultimately, most rules are rules of thumb and should not be followed in cases when they doesn't actually help readability or maintainability. Also, linting is not just utilized in Travis, but also by many editor plugins. It's very annoying if a syntax error is highlighted the same way in my editor as a stylistic error.

[RunDevelopment]

@LeaVerou

I disagree with blanket adoption of "best practices" lists. We should evaluate each of these practices and see if we want it in our project.

You're exactly right. This is what I also intended to do.

It's very annoying if a syntax error is highlighted the same way in my editor as a stylistic error.

That's true. Warning it is then for most stylistic rules. I'd still like to keep things like indent as an error.

[RunDevelopment]

Closed in favor of #1796.