1501931 - Enable ESLint for parser/

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement, P3)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for the parser/ directories.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Please note: We want to end up with two separate commits. One with the automatic changes, the second with the manual changes. It helps with reviewing if these appear as a commit series in phabricator.

Here's some approximate steps:

- In .eslintignore, remove the `parser/**` line.

- Run eslint:

./mach eslint --fix parser

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- Create a commit of the work so far, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for parser/ (automatic changes)" parser/

- For the remaining issues, you'll need to fix them by hand. Most of those should be reasonably easy to understand, but there's more information on some specific bits (especially no-undef) here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ESLint#Common_issues_and_how_to_solve_them
http://eslint.org/docs/rules/

(you can just run `./mach eslint parser` to check you've fixed them).

- Create a second commit of the manual changes, e.g.

$ hg commit -m "Bug nnn - Enable ESLint for parser/ (manual changes)"

- Post the two commits via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
When you post, please request initial review from myself (`standard8`), I'll then redirect to the owners of the code if the changes look good.

Comment 1

[Riccardo Bellingeri]

Having a look at this one

Comment 2

[Riccardo Bellingeri]

Uploaded a patch on Phabricator - https://phabricator.services.mozilla.com/D9921

Comment 3

[Mark Banner (:standard8)]

Riccardo has told me he's no longer working on this, so opening up again.

Comment 4

[r2hkri]

I would like to try this, thanks!

Comment 5

[Mark Banner (:standard8)]

(In reply to r2hkri from comment #4)
> I would like to try this, thanks!

Hi, please do. Feel free to ask here or on irc if you have any questions (https://wiki.mozilla.org/Irc #introduction).

Comment 6

[r2hkri]

I'm running into a problem where a variable is defined in an (x)html file and then referenced inside a loaded script. For example:

https://searchfox.org/mozilla-central/source/parser/htmlparser/tests/mochitest/test_bug688580.html#23
https://searchfox.org/mozilla-central/source/parser/htmlparser/tests/mochitest/file_bug688580.js#2

I tried the suggestions from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ESLint#no-undef and none of them worked or fit the case. I also can't do something like /* globals state */ without also disabling the no-global-assign rule.

Comment 7

[r2hkri]

Attachment: Bug 1501931 - Enable ESLint for parser/ (automatic fixes) r=standard8

Comment 8

[r2hkri]

Attachment: Bug 1501931 - Enable ESLint for parser/ (manual fixes) r=standard8

Depends on D11920

Comment 9

[r2hkri]

Attachment: Bug 1501931 - Enable ESLint for parser/ (manual fixes) r=standard8

Comment 10

[Mark Banner (:standard8)]

Blake, could you have a look at the automatic changes as well please. Although Phabricator is saying "needs revision", I think it is currently confused...

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, I misread the Phabricator queue.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f456b3dd43c0
Enable ESLint for parser/ (automatic fixes) r=Standard8,mrbkap
https://hg.mozilla.org/integration/autoland/rev/7c02d6c28133
Enable ESLint for parser/ (manual fixes) r=Standard8,mrbkap

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f456b3dd43c0
https://hg.mozilla.org/mozilla-central/rev/7c02d6c28133