1251003 - Potentially rename deprecated .eslintrc files

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[:Cykesiopka]

According to http://eslint.org/docs/user-guide/configuring, .eslintrc files are deprecated in favour of .eslintrc.json, .eslintrc.yaml etc.

As far as I can tell, all ESLint config files in m-c are .eslintrc files. Maybe we should rename all the files (either all at once or iteratively)?

Comment 1

[:Cykesiopka]

Mossop: Do you happen to know if there's a reason why we use the deprecated naming style?

Thanks.

Comment 2

[Dave Townsend [:mossop]]

Probably no good reason. I'd happily accept a patch that fixes it.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1251003 - Change .eslintrc files to .eslintrc.js to avoid obsolete config file format.

Review commit: https://reviewboard.mozilla.org/r/85336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85336/

Comment 4

[Mark Banner (:standard8)]

Unfortunately mozreview doesn't do a good job of handling file moves :-(

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Out of curiosity, why did you choose .js instead of .json ? Did one of the files have actual code in it instead of a json-like structure?

Comment 6

[:Cykesiopka]

(In reply to Mark Banner (:standard8) from comment #4)
> Unfortunately mozreview doesn't do a good job of handling file moves :-(

Hmm, did you use |hg mv|? It should represent the moves much nicer if you did.

Comment 7

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> Out of curiosity, why did you choose .js instead of .json ? Did one of the
> files have actual code in it instead of a json-like structure?

.js allows comments, whereas .json does not (at least strictly, and my atom editor also highlights these as invalid). Since a lot of the files have comments in, .js seemed to fit better. It also has the side effect of allowing eslint to lint itself ;-)

(In reply to :Cykesiopka from comment #6)
> (In reply to Mark Banner (:standard8) from comment #4)
> > Unfortunately mozreview doesn't do a good job of handling file moves :-(
> 
> Hmm, did you use |hg mv|? It should represent the moves much nicer if you
> did.

I used git mv with git cinnabar to push, see bug 1264000 for the mozreview issue.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Mark Banner (:standard8) from comment #7)
> (In reply to Philipp Kewisch [:Fallen] from comment #5)
> > Out of curiosity, why did you choose .js instead of .json ? Did one of the
> > files have actual code in it instead of a json-like structure?
> 
> .js allows comments, whereas .json does not (at least strictly, and my atom
> editor also highlights these as invalid). Since a lot of the files have
> comments in, .js seemed to fit better. It also has the side effect of
> allowing eslint to lint itself ;-)

The self-linting is an interesting argument, thanks. Note that eslint's JSON parser understands comments, see the .eslintrc.json files in security/manager.

Comment 9

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen] from comment #8)
> (In reply to Mark Banner (:standard8) from comment #7)
> > (In reply to Philipp Kewisch [:Fallen] from comment #5)
> > > Out of curiosity, why did you choose .js instead of .json ? Did one of the
> > > files have actual code in it instead of a json-like structure?
> > 
> > .js allows comments, whereas .json does not (at least strictly, and my atom
> > editor also highlights these as invalid). Since a lot of the files have
> > comments in, .js seemed to fit better. It also has the side effect of
> > allowing eslint to lint itself ;-)
> 
> The self-linting is an interesting argument, thanks. Note that eslint's JSON
> parser understands comments, see the .eslintrc.json files in
> security/manager.

Yeah, I realised that eslint understands comments, but I was verging on the side of sticking closer to the standards (e.g. if we introduce a json linter then we might not be allowing comments).

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8800433 [details]
Bug 1251003 - Change .eslintrc files to .eslintrc.js to avoid obsolete config file format.

https://reviewboard.mozilla.org/r/85336/#review84234

I can't really review it in this state. Assuming this is just file renames (that are handled correctly in hg) plus the odd update to the extends property then just take a blanket r+. If there is anything else going on here then maybe go old school and attach a patch file to the bug so I can see the changes more clearly.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

If you want to lint .eslintrc.js files, you will need to unignore them. It seems these files are ignored by default. running mach eslint .eslintrc gives you:


/Users/kewisch/mozilla/comm/calendar/.eslintrc.js
  None  warning  File ignored by default.  Use a negated ignore pattern (like "--ignore-pattern '!<relative/path/to/filename>'") to override.  (eslint)


Adding !**/.eslintrc.js to .eslintignore will unignore them. I had a few style issues in ours so be sure to check.

Comment 12

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen] from comment #11)
> If you want to lint .eslintrc.js files, you will need to unignore them. It
> seems these files are ignored by default. running mach eslint .eslintrc
> gives you:
> 
> 
> /Users/kewisch/mozilla/comm/calendar/.eslintrc.js
>   None  warning  File ignored by default.  Use a negated ignore pattern
> (like "--ignore-pattern '!<relative/path/to/filename>'") to override. 
> (eslint)
> 
> 
> Adding !**/.eslintrc.js to .eslintignore will unignore them. I had a few
> style issues in ours so be sure to check.

Thank you for pointing that out Philipp, I've added this to the new patch I'm working on, and fixed up the few issues it raised.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8800433 [details]
Bug 1251003 - Change .eslintrc files to .eslintrc.js to avoid obsolete config file format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85336/diff/1-2/

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8800433 [details]
Bug 1251003 - Change .eslintrc files to .eslintrc.js to avoid obsolete config file format.

Ok, I've now re-done this from a Mercurial based repo & for some reason this seems to have coped with the moves a lot better. Additionally, mozreview seems to cope with the moves as well!

Dave: This is more-or-less file moves, but with additional tops & tails for the files, the new review should show this clearer (even if its a bit slow to load).

There's a few lint fixes for the files now that linting of .eslintrc.js is fixed (thanks to Philipp).

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8800433 [details]
Bug 1251003 - Change .eslintrc files to .eslintrc.js to avoid obsolete config file format.

https://reviewboard.mozilla.org/r/85336/#review85234

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8800433 [details]
Bug 1251003 - Change .eslintrc files to .eslintrc.js to avoid obsolete config file format.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85336/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc2ff940d290
Change .eslintrc files to .eslintrc.js to avoid obsolete config file format. r=mossop

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fc2ff940d290