Potentially rename deprecated .eslintrc files

RESOLVED FIXED in Firefox 52

Status

defect
--
minor
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: Cykesiopka, Assigned: standard8)

Tracking

unspecified
mozilla52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
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)?
Reporter

Comment 1

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

Thanks.
Blocks: eslint
Flags: needinfo?(dtownsend)
Probably no good reason. I'd happily accept a patch that fixes it.
Flags: needinfo?(dtownsend)
Component: General → ESLint
Assignee

Updated

3 years ago
Assignee: nobody → standard8
Assignee

Comment 4

3 years ago
Unfortunately mozreview doesn't do a good job of handling file moves :-(
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?
Reporter

Comment 6

3 years ago
(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.
Assignee

Comment 7

3 years ago
(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.
(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.
Assignee

Comment 9

3 years ago
(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

3 years ago
mozreview-review
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.
Attachment #8800433 - Flags: review?(dtownsend) → review+
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.
Assignee

Comment 12

3 years ago
(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 hidden (mozreview-request)
Assignee

Comment 14

3 years ago
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).
Attachment #8800433 - Flags: review+ → review?(dtownsend)

Comment 15

3 years ago
mozreview-review
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
Attachment #8800433 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)

Comment 17

3 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc2ff940d290
Change .eslintrc files to .eslintrc.js to avoid obsolete config file format. r=mossop

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc2ff940d290
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

Last year
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.