914753 - Remove 'js2-mode' references from Emacs graffiti

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jim Blandy :jimb]

Attachment: Remove 'Mode: js2' markers from DevTools JavaScript files. Emacs has better ways to select the file's mode.

Many of our files have a little comment on the top that says something like:

/* -*- Mode: js2; js2-basic-offset: 2; indent-tabs-mode: nil; -*- */

This is supposed to help Emacs edit the file more appropriately. It may certainly be valuable to tell Emacs about the indentation style, but Emacs can infer the appropriate mode from the file's name. Furthermore, js2-mode isn't distributed with Emacs by default, and many people prefer javascript-mode, which is.

So the effect of "Mode: js2' in this graffiti is to either cause an error message each time the file is visited, or select what many people consider an inferior JavaScript editing mode, when there is a perfectly adequate mechanism for each user to choose whatever mode they like without affecting other devs.

Attached is a patch that removes these marks from the DevTools team's JS files.

Comment 1

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bff7c027874

Comment 2

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6bff7c027874

Comment 3

[Adrian Aichner [:anaran]]

This issue is coming back again.

Should this bug be re-opened?

Should I submit a new one?

I thing the Emacs File Variable bits are still valuable because they also tell other humans which indentation style is being used.

I'll attach a patch for the new js2 stuff showing up in the fx-team.

The bigger issue is incorrect usage of c-basic-offset instead of js-indent-level, which is also fixed in my path.

Comment 4

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-914753-Remove-js2-mode-references-from-Emacs-gra.patch

Comment 5

[Adrian Aichner [:anaran]]

I also like js2-mode, so I have this in my init.el:

(defalias 'js-mode 'js2-mode)
(defvaralias 'js2-basic-offset 'js-indent-level)

See also related discussion at
https://github.com/mooz/js2-mode/issues/144

Comment 6

[Adrian Aichner [:anaran]]

See also new and related (but non-overlapping) Bug 1023839

Comment 7

[Adrian Aichner [:anaran]]

Comment on attachment 8438393 [details] [diff] [review]
0001-Bug-914753-Remove-js2-mode-references-from-Emacs-gra.patch

Based on git blame I would like to request your review of my change.

Best,
Adrian

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8438393 [details] [diff] [review]
0001-Bug-914753-Remove-js2-mode-references-from-Emacs-gra.patch

Sure, looks fine, though in your previous patch you may as well change all the js2-indent-level's to js-indent-level as well.  Also, I admit, I've never seen a local variables line that doesn't include the mode -- I assume that works fine.

Comment 9

[Adrian Aichner [:anaran]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #8)
> Comment on attachment 8438393 [details] [diff] [review]
> 0001-Bug-914753-Remove-js2-mode-references-from-Emacs-gra.patch
> 

Thanks for your feedback, Vladimir!

> Sure, looks fine, though in your previous patch you may as well change all

The previous patch
https://bugzilla.mozilla.org/attachment.cgi?id=802467
isn't mine.

> the js2-indent-level's to js-indent-level as well.  Also, I admit, I've
> never seen a local variables line that doesn't include the mode -- I assume
> that works fine.

I haven't seen that use without a Mode: spec either.
I use Mode: js in my patch.
See also Bug 1023839 which normalizes all the Mode: js|javascript|JavaScript to Mode:js;

fitzgen has given favorable feedback on Bug 1023839.

Comment 10

[Adrian Aichner [:anaran]]

Oh, js2 mode is a lot wider spread that I had initially realized.
This patch would only be a droplet onto a hot stone, to use a German idiom.

Comment 11

[Jim Blandy :jimb]

It's not necessary to have a "mode:" specification in an Emacs file local variable list; it's the "-*-" that draws Emacs's attention to it. So there's no reason to mention any JS mode at all in any of those lines.

Stock Emacs already selects javascript-mode for .js and .json files, and people using Emacs to hack the internals of Firefox can perfectly well add .jsm to their auto-mode-alist themselves:

(add-to-list 'auto-mode-alist '("\\.jsm\\'" . javascript-mode))

Older releases of Emacs did not have entries for JavaScript, which is probably why those lines were added in the first place.

Comment 12

[Jim Blandy :jimb]

(In reply to adrian from comment #3)
> I thing the Emacs File Variable bits are still valuable because they also
> tell other humans which indentation style is being used.

Well, I think people using other editors will probably infer the indentation style from looking at the code.

Comment 13

[Joe Walker [:jwalker]]

(In reply to Jim Blandy :jimb from comment #12)
> (In reply to adrian from comment #3)
> > I thing the Emacs File Variable bits are still valuable because they also
> > tell other humans which indentation style is being used.
> 
> Well, I think people using other editors will probably infer the indentation
> style from looking at the code.

Jim - do I infer from this that you don't benefit much from modelines?

Comment 14

[Adrian Aichner [:anaran]]

(In reply to Jim Blandy :jimb from comment #11)
> It's not necessary to have a "mode:" specification in an Emacs file local
> variable list; it's the "-*-" that draws Emacs's attention to it. So there's

It is necessary if you want to load an Emacs major mode without being at the grace of emacs versions, site-wide or user-specific configurations.

> no reason to mention any JS mode at all in any of those lines.
> 
> Stock Emacs already selects javascript-mode for .js and .json files, and
> people using Emacs to hack the internals of Firefox can perfectly well add
> .jsm to their auto-mode-alist themselves:
> 
> (add-to-list 'auto-mode-alist '("\\.jsm\\'" . javascript-mode))

Yes, I know how to do this, once I know I *need* to do it for some source files, maybe not for others.

> 
> Older releases of Emacs did not have entries for JavaScript, which is
> probably why those lines were added in the first place.

What I was trying to do with my patches is specify the emacs mode configuration more completely, thereby reducing waste for new or infrequent contributors and their reviewers.

Meanwhile I found a set of 79 js files which are hard-wired into j2-mode because they use a feature not present in js-mode: js2-skip-preprocessor-directives

I will not pursue this area any further unless a mozilla reviewer voices interest.

It might be better to concentrate on tools like https://www.npmjs.org/package/jscs which can check for compliance with configurable coding style.

See
https://github.com/mdn/addon-sdk-content-scripts/pull/2
and
https://github.com/mdn/addon-sdk-content-scripts/pull/3

Comment 15

[Adrian Aichner [:anaran]]

(In reply to Jim Blandy :jimb from comment #12)
> (In reply to adrian from comment #3)
> > I thing the Emacs File Variable bits are still valuable because they also
> > tell other humans which indentation style is being used.
> 
> Well, I think people using other editors will probably infer the indentation
> style from looking at the code.

Yes, this being the most ineffective way of cooperation I could think of.

But, hey, I learning a bit looking into this issue, especially how to attribute changes with git blame.

Comment 16

[Jim Blandy :jimb]

(In reply to Joe Walker [:jwalker] from comment #13)
> Jim - do I infer from this that you don't benefit much from modelines?

The modelines in our tree do several different things. Some are beneficial, some not.

I do benefit from modelines when they set the indentation amount correctly for the part of the source tree I'm working in.

Modelines interfere with my work when they set the mode. This overrides my customizations. The benefit it provides others is limited: it's easy to tell Emacs what mode you want to use for .js and .jsm files, and that's the only work the modelines save you.

And finally, I believe the only reasons we have "mode:" settings at all is that for years Emacs had no JavaScript mode at all. Java mode and C++ mode were better than nothing. When Emacs did get a JS mode, I suspect people changed "mode: C++" to "mode: JS" because they weren't sure about deleting it altogether.

Comment 17

[Jim Blandy :jimb]

(In reply to adrian from comment #14)
> (In reply to Jim Blandy :jimb from comment #11)
> > It's not necessary to have a "mode:" specification in an Emacs file local
> > variable list; it's the "-*-" that draws Emacs's attention to it. So there's
> 
> It is necessary if you want to load an Emacs major mode without being at the
> grace of emacs versions, site-wide or user-specific configurations.

It doesn't make sense to have the files override the Emacs user's own settings for how JS files should be managed. There is nothing so special about FF's JS that we can't trust the user to choose the JS mode they like.

And there's certainly no reason to explicitly set the mode to what Emacs would choose by default. This has *no* effect beyond frustrating the Emacs user.

Yes, older Emacs versions had bad JS modes, or none. Now all Linux distributions and MacPorts have Emacs versions that include js.el, which is perfectly workable. That old versions behave poorly not a compelling reason to be overriding the preferences of the majority of developers who are running something recent enough.


> Yes, I know how to do this, once I know I *need* to do it for some source
> files, maybe not for others.

Do you have a compelling need to use different JavaScript modes on different areas of the tree? This seems really unlikely.

Comment 18

[Jim Blandy :jimb]

Attachment: Fix a broken Emacs mode line.

Comment 19

[Jim Blandy :jimb]

Attachment: Remove Emacs mode settings and unnecessary tab width settings, and fix indent level settings.

Comment 20

[Jim Blandy :jimb]

Attachment: Completely over-engineered Python script to fix Emacs mode lines

Here's the script that generated that patch. It might be more useful to review the script than to review the patch.

Comment 21

[Adrian Aichner [:anaran]]

Attachment: blame-js2-skip-preprocessor-directives-20140613T205835+0200-author-frequency.txt

@jimb Perhaps you want to talk to some of the js2-skip-preprocessor-directives line authors before you perform such a sweeping change.

I created the file with
history entry
       date                 command
 3968  20140613T205835+0200 (for i in `grep -l --include=*.jsm --include=*.js -r -i -E "js2-skip-preprocessor-directives:" --exclude-dir=firefox-static --exclude-dir=obj-i686-pc-* .`; do git blame -L /js2-skip-preprocessor-directives:/,+0 -e HEAD -- $i; done) > blame-js2-skip-preprocessor-directives-`dtz`.txt
 3984  20140614T134938+0200 sed -n -r -e "s/.+<([^>]+)>.+js2.+/\1/p" blame-js2-skip-preprocessor-directives-20140613T205835+0200.txt | sort | uniq -c | sort > blame-js2-skip-preprocessor-directives-20140613T205835+0200-author-frequency.txt
in gecko-dev on git branch fx-team.

Comment 22

[Jim Blandy :jimb]

Who in the world can review this? It doesn't actually touch any JS code, so it's NPOB. It doesn't seem worth it to r? all those module owners individually.

Comment 23

[Jim Blandy :jimb]

(In reply to adrian from comment #21)
> @jimb Perhaps you want to talk to some of the
> js2-skip-preprocessor-directives line authors before you perform such a
> sweeping change.

Thanks, this is great. I'll consult.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jim Blandy :jimb from comment #22)
> Who in the world can review this? It doesn't actually touch any JS code, so
> it's NPOB. It doesn't seem worth it to r? all those module owners
> individually.

At least for devtools, if you want to split that part out we can just land it. Here: r+ :)

Maybe gavin could review for everything?

Comment 25

[Jim Blandy :jimb]

Jim, are you the person who added the js2-skip-preprocessor-directives settings to the M-C JS files? It's not a problem, but we'd prefer not to accumulate stuff people don't need any more. Are those still valuable to you?

Comment 26

[Jim Blandy :jimb]

Attachment: Remove Emacs mode settings and unnecessary tab width settings, and fix indent level settings, but leave in js2-skip-preprocessor-directives.

Here's a version that leaves js2-skip-preprocessor-directives alone, so that question can be considered separately.

Comment 27

[Jim Blandy :jimb]

It also adds indent-tabs-mode: nil everywhere, since it's present in all but 25 files anyway.

Comment 28

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

This bug is marked Resolved/Fixed - is review still needed on those patches?

Comment 29

[Jim Blandy :jimb]

I think its scope has expanded. Re-opening.

Comment 30

[Jim Mathies [:jimm]]

Why was I ni'd on this?

Comment 31

[Jim Mathies [:jimm]]

(In reply to Jim Blandy :jimb from comment #25)
> Jim, are you the person who added the js2-skip-preprocessor-directives
> settings to the M-C JS files? It's not a problem, but we'd prefer not to
> accumulate stuff people don't need any more. Are those still valuable to you?

certainly not me! :) Sorry I missed this.

Comment 32

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8438393 [details] [diff] [review]
0001-Bug-914753-Remove-js2-mode-references-from-Emacs-gra.patch

Review of attachment 8438393 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!  Thanks for bringing us back into modernity! :)

Comment 33

[Jim Blandy :jimb]

Attachment: Fix broken Emacs mode lines.

I'm picking Ehsan as a reviewer for this, for his build system background and overall-tree-hacking background. Ehsan, is there someone better I should ask instead?

Comment 34

[Jim Blandy :jimb]

Attachment: Make Emacs file variable header lines correct, or at least consistent.

[Instead of reviewing this patch, you probably want to review the Python script that generated it; it is attached to the bug.]

The -*- file variable lines -*- establish per-file settings that Emacs will
pick up. This patch makes the following changes to those lines (and touches
nothing else):

 - Never set the buffer's mode.

   Years ago, Emacs did not have a good JavaScript mode, so it made sense
   to use Java or C++ mode in .js files. However, Emacs has had js-mode for
   years now; it's perfectly serviceable, and is available and enabled by
   default in all major Emacs packagings.

   Selecting a mode in the -*- file variable line -*- is almost always the
   wrong thing to do anyway. It overrides Emacs's default choice, which is
   (now) reasonable; and even worse, it overrides settings the user might
   have made in their '.emacs' file for that file extension. It's only
   useful when there's something specific about that particular file that
   makes a particular mode appropriate.

 - Correctly propagate settings that establish the correct indentation
   level for this file: c-basic-offset and js2-basic-offset should be
   js-indent-level. Whatever value they're given should be preserved;
   different parts of our tree use different indentation styles.

 - We don't use tabs in Mozilla JS code. Always set indent-tabs-mode: nil.
   Remove tab-width: settings, at least in files that don't contain tab
   characters.

 - Remove js2-mode settings that belong in the user's .emacs file, like
   js2-skip-preprocessor-directives.

Comment 35

[Jim Blandy :jimb]

Attachment: Python script to fix Emacs -*- file variable -*- lines.

Comment 36

[(no longer active)]

Comment on attachment 8444825 [details] [diff] [review]
Make Emacs file variable header lines correct, or at least consistent.

Review of attachment 8444825 [details] [diff] [review]:
-----------------------------------------------------------------

r=me based on a quick skim over https://bug914753.bugzilla.mozilla.org/attachment.cgi?id=8444826.  Note that I don't use Emacs so I'm basically trusting you on these changes being desirable.  :-)

Comment 37

[Jim Blandy :jimb]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc1d2feb735
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/69d61e42d5df

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3dc1d2feb735
https://hg.mozilla.org/mozilla-central/rev/69d61e42d5df

Comment 39

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/62d9ecc8dd8c8785bdfa52b7123a4f4cf02a150d
Bug 914753: Make Emacs file variable header lines correct, or at least consistent. DONTBUILD r=ehsan

The -*- file variable lines -*- establish per-file settings that Emacs will
pick up. This patch makes the following changes to those lines (and touches
nothing else):

- Never set the buffer's mode.

Years ago, Emacs did not have a good JavaScript mode, so it made sense
to use Java or C++ mode in .js files. However, Emacs has had js-mode for
years now; it's perfectly serviceable, and is available and enabled by
default in all major Emacs packagings.

Selecting a mode in the -*- file variable line -*- is almost always the
wrong thing to do anyway. It overrides Emacs's default choice, which is
(now) reasonable; and even worse, it overrides settings the user might
have made in their '.emacs' file for that file extension. It's only
useful when there's something specific about that particular file that
makes a particular mode appropriate.

- Correctly propagate settings that establish the correct indentation
level for this file: c-basic-offset and js2-basic-offset should be
js-indent-level. Whatever value they're given should be preserved;
different parts of our tree use different indentation styles.

- We don't use tabs in Mozilla JS code. Always set indent-tabs-mode: nil.
Remove tab-width: settings, at least in files that don't contain tab
characters.

- Remove js2-mode settings that belong in the user's .emacs file, like
js2-skip-preprocessor-directives.

Comment 40

[David Dahl :ddahl]

Comment on attachment 8438393 [details] [diff] [review]
0001-Bug-914753-Remove-js2-mode-references-from-Emacs-gra.patch

I should probably drik coffee before operating a <select>. I just meant to clear it:)