Closed Bug 1251003 Opened 5 years ago Closed 4 years ago

Potentially rename deprecated .eslintrc files

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set
minor

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: Cykesiopka, Assigned: standard8)

References

Details

Attachments

(1 file)

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)?
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: nobody → standard8
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?
(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.
(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.
(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 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.
(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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/fc2ff940d290
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.