Closed Bug 914753 Opened 6 years ago Closed 5 years ago

Remove 'js2-mode' references from Emacs graffiti

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(6 files, 4 obsolete files)

4.54 KB, patch
dcamp
: review+
jimb
: checkin+
Details | Diff | Splinter Review
2.87 KB, patch
jedp
: review+
vlad
: review+
Details | Diff | Splinter Review
540 bytes, text/plain
Details
3.32 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.14 MB, patch
ehsan
: review+
Details | Diff | Splinter Review
6.74 KB, text/plain
Details
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.
Attachment #802467 - Flags: review?(dcamp)
Attachment #802467 - Flags: review?(dcamp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bff7c027874
Assignee: nobody → jimb
Flags: in-testsuite-
Target Milestone: --- → Firefox 26
https://hg.mozilla.org/mozilla-central/rev/6bff7c027874
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
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
See also new and related (but non-overlapping) Bug 1023839
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
Attachment #8438393 - Flags: review?(vladimir)
Attachment #8438393 - Flags: review?(jparsons)
Attachment #8438393 - Flags: review?(david)
Attachment #8438393 - Flags: review?(andres)
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.
Attachment #8438393 - Flags: review?(vladimir) → review+
(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.
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.
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.
(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.
(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?
(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
(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.
(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.
(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.
Attached patch Fix a broken Emacs mode line. (obsolete) — Splinter Review
Attachment #802467 - Flags: checkin+
Here's the script that generated that patch. It might be more useful to review the script than to review the patch.
@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.
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.
(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.
(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?
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?
Flags: needinfo?(jmathies)
Here's a version that leaves js2-skip-preprocessor-directives alone, so that question can be considered separately.
It also adds indent-tabs-mode: nil everywhere, since it's present in all but 25 files anyway.
This bug is marked Resolved/Fixed - is review still needed on those patches?
Flags: needinfo?(jimb)
I think its scope has expanded. Re-opening.
Status: RESOLVED → REOPENED
Flags: needinfo?(jimb)
Resolution: FIXED → ---
Whiteboard: [leave open]
Why was I ni'd on this?
Flags: needinfo?(jmathies)
(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 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! :)
Attachment #8438393 - Flags: review?(jparsons) → review+
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?
Attachment #8440962 - Attachment is obsolete: true
Attachment #8444823 - Flags: review?(ehsan)
[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.
Attachment #8440963 - Attachment is obsolete: true
Attachment #8441016 - Attachment is obsolete: true
Attachment #8444825 - Flags: review?(ehsan)
Attachment #8440978 - Attachment is obsolete: true
Attachment #8444823 - Flags: review?(ehsan) → review+
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.  :-)
Attachment #8444825 - Flags: review?(ehsan) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc1d2feb735
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/69d61e42d5df
Whiteboard: [leave open]
Target Milestone: Firefox 26 → Firefox 33
https://hg.mozilla.org/mozilla-central/rev/3dc1d2feb735
https://hg.mozilla.org/mozilla-central/rev/69d61e42d5df
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
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.
Attachment #8438393 - Flags: review?(andres)
Attachment #8438393 - Flags: review?(david) → review-
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:)
Attachment #8438393 - Flags: review-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.