Use js-indent-level in .js, .jsm emacs file variables, standardize on mode: js

UNCONFIRMED
Assigned to

Status

()

Firefox
Developer Tools
UNCONFIRMED
4 years ago
3 months ago

People

(Reporter: anaran, Assigned: jimb)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Usage of c-basic-offset instead of js-indent-level won't work on a per-file basis.

js-mode ignores it, js2-mode only initializes from it when the mode is loaded.

I also like js2-mode, so I have the following in my init.el to use js2-mode, even for files specifying js-mode, still obeying the specified js-indent-level.

(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

I'll attach a proposed patch shortly.
(Reporter)

Comment 1

4 years ago
Created attachment 8438397 [details] [diff] [review]
0001-Bug-1023839-Use-js-indent-level-in-.js-.jsm-emacs-fi.patch
(Reporter)

Comment 2

4 years ago
Created attachment 8439793 [details] [diff] [review]
0001-Bug-1023839-Use-js-indent-level-in-.js-.jsm-emacs-fi.patch

Fixes four files where the substitution resulted in duplication of "js-indent-level: 2; ".
I used git blame to find the most recent authors of the emacs file variable lines, based on the tip of branch fx-team.
The top three contributors touched 72 of the 186 files affected.
I have asked them for review of this patch.
Attachment #8438397 - Attachment is obsolete: true
Attachment #8439793 - Flags: review?(vporof)
Attachment #8439793 - Flags: review?(paul)
Attachment #8439793 - Flags: review?(bgrinstead)
(Reporter)

Comment 3

4 years ago
See Bug 914753 for the context of my js2-mode remark, which is lacking context here without that background.
Comment on attachment 8439793 [details] [diff] [review]
0001-Bug-1023839-Use-js-indent-level-in-.js-.jsm-emacs-fi.patch

I don't use emacs much these days, so I'm not the best person to assess if these changes are warranted or not.
Attachment #8439793 - Flags: review?(vporof)
Comment on attachment 8439793 [details] [diff] [review]
0001-Bug-1023839-Use-js-indent-level-in-.js-.jsm-emacs-fi.patch

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

I'm in the same boat as Victor.  Maybe fitzgen has some feedback about this?
Attachment #8439793 - Flags: review?(bgrinstead) → feedback?(nfitzgerald)
Attachment #8439793 - Flags: feedback?(nfitzgerald) → feedback+
(Assignee)

Comment 6

4 years ago
There's no need to say mode: js in any of these files. See:
https://bugzilla.mozilla.org/show_bug.cgi?id=914753#c11

Comment 7

3 years ago
These days I think it would be better to remove (nearly) all of the Emacs cookies from
the source and replace them with a single .dir-locals.el file.  The only ones that would
have to remain are where choosing the mode is appropriate -- e.g., moz.build, or maybe
.h files generally.

.dir-locals.el was added in Emacs 23, released in 2009.

I'll attach the file I'm using now.

Comment 8

3 years ago
Created attachment 8490934 [details]
.dir-locals.el

Updated

3 years ago
Attachment #8439793 - Flags: review?(paul)
(Assignee)

Comment 9

3 months ago
I'll push this through. Tom, is this still the .dir-locals.el file you recommend?
Assignee: nobody → jimb
Flags: needinfo?(ttromey)

Comment 10

3 months ago
I'm using this now:

(
 ;; Generic settings.
 (nil .
      ;; See C-h f bug-reference-prog-mode, e.g, for using this.
      ((bug-reference-url-format . "https://bugzilla.mozilla.org/show_bug.cgi?id=%s")
       (bug-reference-bug-regexp . "\\([Bb]ug ?#?\\|[Pp]atch ?#\\|RFE ?#\\|PR [a-z-+]+/\\)\\([0-9]+\\(?:#[0-9]+\\)?\\)")))

 ;; The built-in javascript mode.
 (js-mode .
     ((indent-tabs-mode . nil)
      (js-indent-level . 2)
      (js-switch-indent-offset . 2)))

 (c++-mode .
	   ((indent-tabs-mode . nil)
	    (c-basic-offset . 2)))

 (idl-mode .
	   ((indent-tabs-mode . nil)
	    (c-basic-offset . 2)))

 (css-mode .
	   ((indent-tabs-mode . nil)
	    (css-indent-offset . 2)))

 ;; New in 26.
 (mhtml-mode .
     ((indent-tabs-mode . nil)
      (js-indent-level . 2)
      (js-switch-indent-offset . 2)
      (css-indent-offset . 2)))
)


Probably not truly correct for the whole tree.
Flags: needinfo?(ttromey)
You need to log in before you can comment on or make changes to this bug.