Potentially rename deprecated .eslintrc files

RESOLVED FIXED in Firefox 52

Status

Testing
Lint
--
minor
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Cykesiopka, Assigned: standard8)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 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

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

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

Updated

a year ago
Assignee: nobody → standard8
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc2ff940d290
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.