Closed
Bug 914753
Opened 11 years ago
Closed 10 years ago
Remove 'js2-mode' references from Emacs graffiti
Categories
(DevTools :: General, defect)
DevTools
General
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.akhgari
:
review+
|
Details | Diff | Splinter Review |
2.14 MB,
patch
|
ehsan.akhgari
:
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)
Updated•11 years ago
|
Attachment #802467 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 1•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bff7c027874
Assignee: nobody → jimb
Flags: in-testsuite-
Target Milestone: --- → Firefox 26
Comment 2•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6bff7c027874
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 3•10 years ago
|
||
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•10 years ago
|
||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
See also new and related (but non-overlapping) Bug 1023839
Comment 7•10 years ago
|
||
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+
Comment 9•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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.
Assignee | ||
Comment 12•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #802467 -
Flags: checkin+
Assignee | ||
Comment 20•10 years ago
|
||
Here's the script that generated that patch. It might be more useful to review the script than to review the patch.
Comment 21•10 years ago
|
||
@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.
Assignee | ||
Comment 22•10 years ago
|
||
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.
Assignee | ||
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
(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?
Assignee | ||
Comment 25•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jmathies)
Assignee | ||
Comment 26•10 years ago
|
||
Here's a version that leaves js2-skip-preprocessor-directives alone, so that question can be considered separately.
Assignee | ||
Comment 27•10 years ago
|
||
It also adds indent-tabs-mode: nil everywhere, since it's present in all but 25 files anyway.
Comment 28•10 years ago
|
||
This bug is marked Resolved/Fixed - is review still needed on those patches?
Flags: needinfo?(jimb)
Assignee | ||
Comment 29•10 years ago
|
||
I think its scope has expanded. Re-opening.
Status: RESOLVED → REOPENED
Flags: needinfo?(jimb)
Resolution: FIXED → ---
Whiteboard: [leave open]
Comment 31•10 years ago
|
||
(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•10 years ago
|
||
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+
Assignee | ||
Comment 33•10 years ago
|
||
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)
Assignee | ||
Comment 34•10 years ago
|
||
[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)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8440978 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8444823 -
Flags: review?(ehsan) → review+
Comment 36•10 years ago
|
||
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+
Assignee | ||
Comment 37•10 years ago
|
||
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
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3dc1d2feb735 https://hg.mozilla.org/mozilla-central/rev/69d61e42d5df
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Comment 39•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8438393 -
Flags: review?(andres)
Updated•10 years ago
|
Attachment #8438393 -
Flags: review?(david) → review-
Comment 40•10 years ago
|
||
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-
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•