Requesting patch review

Requesting patch review

[Jackson Hamilton]

[-- Attachment #1.1: Type: text/plain, Size: 123 bytes --]

Hi guys,

New patch for js-mode. Adds a new indentation option. I would appreciate
review before merging.

Thanks,
Jackson

[-- Attachment #1.2: Type: text/html, Size: 202 bytes --]

[-- Attachment #2: 0001-New-indentation-option-for-js-mode.patch --]
[-- Type: text/x-patch, Size: 4144 bytes --]

From 4915e411a22eeefc9561639f5cde2bbdf1731c00 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Sat, 7 Mar 2015 18:01:05 -0800
Subject: [PATCH] New indentation option for js-mode

* lisp/progmodes/js.el (js--proper-indentation): Add and use custom
option `js-dynamic-multi-line-declaration-indentation'.
---
 lisp/ChangeLog       |  5 +++++
 lisp/progmodes/js.el | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 0b277c7..2609693 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-03-07  Jackson Ray Hamilton  <jackson@jacksonrayhamilton.com>
+
+	* lisp/progmodes/js.el (js--proper-indentation): Add and use new
+	custom option `js-dynamic-multi-line-declaration-indentation'.
+
 2015-03-07  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* battery.el (battery-echo-area-format): Simplify default.
diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index d7712e4..2f00da0 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -509,6 +509,37 @@ getting timeout messages."
   :type 'integer
   :group 'js)
 
+(defcustom js-dynamic-multi-line-declaration-indentation nil
+  "Non-nil to indent the first function, array or object literal
+in a multi-line variable declaration specially.
+
+Normally, the first item is unindented, and subsequent items have
+their identifiers lined up against the first:
+
+  var o = {
+      foo: 3
+  };
+
+  var o = {
+      foo: 3
+  },
+      bar = 2;
+
+With this option enabled, the first declaration will be indented
+if there are more declarations, such that the first function,
+array or object lines up nicely with the remaining declarations:
+
+  var o = {
+      foo: 3
+  };
+
+  var o = {
+          foo: 3
+      },
+      bar = 2;"
+  :type 'boolean
+  :group 'js)
+
 ;;; KeyMap
 
 (defvar js-mode-map
@@ -1884,13 +1915,34 @@ In particular, return the buffer position of the first `for' kwd."
 	   ;; "case" and "default".
            (let ((same-indent-p (looking-at "[]})]"))
                  (switch-keyword-p (looking-at "default\\_>\\|case\\_>[^:]"))
-                 (continued-expr-p (js--continued-expression-p)))
+                 (continued-expr-p (js--continued-expression-p))
+                 declaration-keyword-end)
              (goto-char (nth 1 parse-status)) ; go to the opening char
              (if (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
                  (progn ; nothing following the opening paren/bracket
                    (skip-syntax-backward " ")
                    (when (eq (char-before) ?\)) (backward-list))
                    (back-to-indentation)
+                   (when (and js-dynamic-multi-line-declaration-indentation
+                              ;; Check the condition and
+                              ;; simultaneously preserve the match
+                              ;; data.
+                              (when (looking-at js--declaration-keyword-re)
+                                (setq declaration-keyword-end (match-end 0)))
+                              ;; Determine if the declaration
+                              ;; statement has multiple declarations
+                              ;; (implied if at least one is separated
+                              ;; by a ",").
+                              (save-excursion
+                                (goto-char (nth 1 parse-status))
+                                (when (condition-case nil
+                                          (progn
+                                            (forward-sexp)
+                                            t)
+                                        (error nil))
+                                  (while (forward-comment 1))
+                                  (looking-at ","))))
+                     (goto-char (1+ declaration-keyword-end)))
                    (let* ((in-switch-p (unless same-indent-p
                                          (looking-at "\\_<switch\\_>")))
                           (same-indent-p (or same-indent-p
-- 
1.9.1

Re: Requesting patch review

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

() Jackson Hamilton <jackson@jacksonrayhamilton.com>
() Sat, 7 Mar 2015 18:09:01 -0800

   +	* lisp/progmodes/js.el (js--proper-indentation): Add and use new
   +	custom option `js-dynamic-multi-line-declaration-indentation'.

I haven't looked at the code; this comment is on the ChangeLog,
which mentions ‘js-dynamic-multi-line-declaration-indentation’
(good), but in the wrong place (bad).  Better something like:

 * lisp/progmodes/js.el
 (js-dynamic-multi-line-declaration-indentation): New custom option.
 (js--proper-indentation): Use it.

That's admittedly more ugly, but maybe that could be a reason to
find a more concise name for the new custom option...

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Requesting patch review

[Dmitry Gutov]

On 03/08/2015 04:09 AM, Jackson Hamilton wrote:

> New patch for js-mode. Adds a new indentation option. I would appreciate
> review before merging.

Thanks for the patch.

I can see two directions for it to be improved:

- Port the tests. I suppose test/indent/*.js would be a good place for 
the examples. You can create js-???.js, set the new option's, value 
using file local variables (in a comment, at the top or the bottom of 
the file), and then the right indentation would be tested automatically. 
Run a single file with 'make js.js.test'.

- Add the option for the user to always have this indentation, no matter 
if there's a comma after the first item or not. js2-mode can do that 
(here's the original feature request: 
https://github.com/mooz/js2-mode/issues/3). I suppose it'd be best if 
the new variable had a different name and 3 possible values (nil - 
default, t - always, dynamic - check to see if there are several 
declarations).

By the way, in general it's better to send patches to the bug tracker. 
They can get lost in emacs-devel if nobody pays attention right away.

Re: Requesting patch review

[Jackson Hamilton]

[-- Attachment #1.1: Type: text/plain, Size: 1654 bytes --]

For the sake of continuity I'll send my next patch through this email
thread, but in the future I will send them to the bug tracker.

Refactored the code to be less tricky, added the additional level of
configuration and ported the tests.

(Note that I had to `cd test/indent` and use `make js.js.test
EMACS='../../src/emacs -Q'` to get the tests to run in a clean and correct
environment. We may want to update the Makefile to do that by default.

On Sun, Mar 8, 2015 at 10:55 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 03/08/2015 04:09 AM, Jackson Hamilton wrote:
>
>  New patch for js-mode. Adds a new indentation option. I would appreciate
>> review before merging.
>>
>
> Thanks for the patch.
>
> I can see two directions for it to be improved:
>
> - Port the tests. I suppose test/indent/*.js would be a good place for the
> examples. You can create js-???.js, set the new option's, value using file
> local variables (in a comment, at the top or the bottom of the file), and
> then the right indentation would be tested automatically. Run a single file
> with 'make js.js.test'.
>
> - Add the option for the user to always have this indentation, no matter
> if there's a comma after the first item or not. js2-mode can do that
> (here's the original feature request: https://github.com/mooz/js2-
> mode/issues/3). I suppose it'd be best if the new variable had a
> different name and 3 possible values (nil - default, t - always, dynamic -
> check to see if there are several declarations).
>
> By the way, in general it's better to send patches to the bug tracker.
> They can get lost in emacs-devel if nobody pays attention right away.
>

[-- Attachment #1.2: Type: text/html, Size: 2273 bytes --]

[-- Attachment #2: 0001-New-indentation-option-for-js-mode.patch --]
[-- Type: text/x-patch, Size: 7258 bytes --]

From d441bcbfdd9920bba9b5abaec51df1669b882333 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Sat, 7 Mar 2015 18:01:05 -0800
Subject: [PATCH] New indentation option for js-mode

* lisp/progmodes/js.el (js--proper-indentation): Add new custom option
`js-indent-first-initialiser'.

* test/indent/js.js: Add local variables.

* test/indent/js-indent-first-initialiser-t.js: New test for
`js-indent-first-initialiser'.

* test/indent/js-indent-first-initialiser-dynamic.js: New test for
`js-indent-first-initialiser'.
---
 lisp/ChangeLog                                     | 13 ++++
 lisp/progmodes/js.el                               | 68 +++++++++++++++++++
 test/indent/js-indent-first-initialiser-dynamic.js | 78 ++++++++++++++++++++++
 test/indent/js-indent-first-initialiser-t.js       | 69 +++++++++++++++++++
 test/indent/js.js                                  |  7 +-
 5 files changed, 233 insertions(+), 2 deletions(-)
 create mode 100644 test/indent/js-indent-first-initialiser-dynamic.js
 create mode 100644 test/indent/js-indent-first-initialiser-t.js

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 5f26239..f5dacf0 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,16 @@
+2015-03-08  Jackson Ray Hamilton  <jackson@jacksonrayhamilton.com>
+
+	* lisp/progmodes/js.el (js--proper-indentation): Add new custom
+	option `js-indent-first-initialiser'.
+
+	* test/indent/js.js: Add local variables.
+
+	* test/indent/js-indent-first-initialiser-t.js: New test for
+	`js-indent-first-initialiser'.
+
+	* test/indent/js-indent-first-initialiser-dynamic.js: New test for
+	`js-indent-first-initialiser'.
+
 2015-03-08  Dmitry Gutov  <dgutov@yandex.ru>
 
 	* progmodes/ruby-mode.el (ruby-font-lock-keywords): Use
diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index d7712e4..879e3fb 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -509,6 +509,50 @@ getting timeout messages."
   :type 'integer
   :group 'js)
 
+(defcustom js-indent-first-initialiser nil
+  "Specially indent the first variable declaration's initialiser
+in variable statements.
+
+Normally, the first declaration's initialiser is unindented, and
+subsequent declarations have their identifiers lined up against
+the first:
+
+  var o = {
+      foo: 3
+  };
+
+  var o = {
+      foo: 3
+  },
+      bar = 2;
+
+When t, always indent the first declaration's initialiser by an
+additional level:
+
+  var o = {
+          foo: 3
+      };
+
+  var o = {
+          foo: 3
+      },
+      bar = 2;
+
+When `dynamic', if there is only one declaration, don't indent
+the first one's initialiser; otherwise, indent it.
+
+  var o = {
+      foo: 3
+  };
+
+  var o = {
+          foo: 3
+      },
+      bar = 2;"
+  :type 'boolean
+  :safe 'symbolp
+  :group 'js)
+
 ;;; KeyMap
 
 (defvar js-mode-map
@@ -1891,6 +1935,30 @@ In particular, return the buffer position of the first `for' kwd."
                    (skip-syntax-backward " ")
                    (when (eq (char-before) ?\)) (backward-list))
                    (back-to-indentation)
+                   (cond
+                    ((eq js-indent-first-initialiser t)
+                     (when (looking-at js--declaration-keyword-re)
+                       (goto-char (1+ (match-end 0)))))
+                    ((eq js-indent-first-initialiser 'dynamic)
+                     (let ((bracket (nth 1 parse-status))
+                           declaration-keyword-end
+                           at-closing-bracket-p
+                           comma-p)
+                       (when (looking-at js--declaration-keyword-re)
+                         (setq declaration-keyword-end (match-end 0))
+                         (save-excursion
+                           (goto-char bracket)
+                           (setq at-closing-bracket-p
+                                 (condition-case nil
+                                     (progn
+                                       (forward-sexp)
+                                       t)
+                                   (error nil)))
+                           (when at-closing-bracket-p
+                             (while (forward-comment 1))
+                             (setq comma-p (looking-at-p ","))))
+                         (when comma-p
+                           (goto-char (1+ declaration-keyword-end)))))))
                    (let* ((in-switch-p (unless same-indent-p
                                          (looking-at "\\_<switch\\_>")))
                           (same-indent-p (or same-indent-p
diff --git a/test/indent/js-indent-first-initialiser-dynamic.js b/test/indent/js-indent-first-initialiser-dynamic.js
new file mode 100644
index 0000000..a0e1ad1
--- /dev/null
+++ b/test/indent/js-indent-first-initialiser-dynamic.js
@@ -0,0 +1,78 @@
+var foo = function() {
+  return 7;
+};
+
+var foo = function() {
+      return 7;
+    },
+    bar = 8;
+
+var foo = function() {
+      return 7;
+    },
+    bar = function() {
+      return 8;
+    };
+
+var foo = [
+  7
+];
+
+var foo = [
+      7
+    ],
+    bar = 8;
+
+var foo = [
+      7
+    ],
+    bar = [
+      8
+    ];
+
+var o = {
+  foo: 3
+};
+
+var o = {
+      foo: 3
+    },
+    bar = 2;
+
+var o = {
+      foo: 3
+    },
+    bar = {
+      baz: 2
+    };
+
+const o = {
+  foo: 3
+};
+
+const o = {
+        foo: 3
+      },
+      bar = 2;
+
+const o = {
+        foo: 3
+      },
+      bar = {
+        baz: 2
+      };
+
+// Local Variables:
+// indent-tabs-mode: nil
+// js-indent-level: 2
+// js-indent-first-initialiser: dynamic
+// End:
+
+// The following test intentionally produces a scan error and should
+// be placed below all other tests to prevent awkward indentation.
+// (It still thinks it's within the body of a function.)
+
+var foo = function() {
+  return 7;
+  ,
+  bar = 8;
diff --git a/test/indent/js-indent-first-initialiser-t.js b/test/indent/js-indent-first-initialiser-t.js
new file mode 100644
index 0000000..b026503
--- /dev/null
+++ b/test/indent/js-indent-first-initialiser-t.js
@@ -0,0 +1,69 @@
+var foo = function() {
+      return 7;
+    };
+
+var foo = function() {
+      return 7;
+    },
+    bar = 8;
+
+var foo = function() {
+      return 7;
+    },
+    bar = function() {
+      return 8;
+    };
+
+var foo = [
+      7
+    ];
+
+var foo = [
+      7
+    ],
+    bar = 8;
+
+var foo = [
+      7
+    ],
+    bar = [
+      8
+    ];
+
+var o = {
+      foo: 3
+    };
+
+var o = {
+      foo: 3
+    },
+    bar = 2;
+
+var o = {
+      foo: 3
+    },
+    bar = {
+      baz: 2
+    };
+
+const o = {
+        foo: 3
+      };
+
+const o = {
+        foo: 3
+      },
+      bar = 2;
+
+const o = {
+        foo: 3
+      },
+      bar = {
+        baz: 2
+      };
+
+// Local Variables:
+// indent-tabs-mode: nil
+// js-indent-level: 2
+// js-indent-first-initialiser: t
+// End:
diff --git a/test/indent/js.js b/test/indent/js.js
index f41849d..ad7cb56 100644
--- a/test/indent/js.js
+++ b/test/indent/js.js
@@ -1,5 +1,3 @@
-// -*- js-indent-level: 2 -*-
-
 var a = 1;
 b = 2;
 
@@ -65,3 +63,8 @@ b +=
 
 baz(`http://foo.bar/${tee}`)
   .qux();
+
+// Local Variables:
+// indent-tabs-mode: nil
+// js-indent-level: 2
+// End:
-- 
1.9.1

Re: Requesting patch review

[Dmitry Gutov]

On 03/09/2015 12:01 AM, Jackson Hamilton wrote:
> For the sake of continuity I'll send my next patch through this email
> thread, but in the future I will send them to the bug tracker.

Exactly what I was thinking.

> Refactored the code to be less tricky, added the additional level of
> configuration and ported the tests.

Thanks! That's a bit too many test examples for my taste (especially in 
js-indent-first-initialiser-t.js, since that piece of implementation is 
relatively trivial), but I guess that's still much better than no tests.

Regarding the meat of the change, it should be "initializer" (Emacs 
prefers US spelling), and maybe we should also extract the added lines 
into a utility function, in order not to make `js--proper-indentation' 
even longer than it is now.

Running test/indent examples, Was: Re: Requesting patch review

[Dmitry Gutov]

On 03/09/2015 12:01 AM, Jackson Hamilton wrote:

> (Note that I had to `cd test/indent` and use `make js.js.test
> EMACS='../../src/emacs -Q'` to get the tests to run in a clean and
> correct environment. We may want to update the Makefile to do that by
> default.

That's what I did too (aside from -Q, which seems unnecessary because of 
--batch).

I've pushed an update which should address that (and 'make' should run 
all examples now). Not sure how portable it is.

Ideally, we should also run these examples from 'make check', but a lot 
of them actually fail right now (nxml.xml, pascal.pas, scheme.scm; 
probably has to do with the default values of some indentation variables).

Re: Requesting patch review

[Jackson Hamilton]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

> Regarding the meat of the change, it should be "initializer" (Emacs
prefers US spelling)

I used that spelling because that's what ECMAScript uses; from
http://www.ecma-international.org/ecma-262/5.1/#sec-12.2:

> VariableDeclaration : Identifier Initialiser opt

(Dunno who has priority here, Emacs or the spec, but I'm fine with changing
it.)

> maybe we should also extract the added lines into a utility function

How does `js--maybe-goto-declaration-keyword-end` sound?

On Mon, Mar 9, 2015 at 4:36 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 03/09/2015 12:01 AM, Jackson Hamilton wrote:
>
>> For the sake of continuity I'll send my next patch through this email
>> thread, but in the future I will send them to the bug tracker.
>>
>
> Exactly what I was thinking.
>
>  Refactored the code to be less tricky, added the additional level of
>> configuration and ported the tests.
>>
>
> Thanks! That's a bit too many test examples for my taste (especially in
> js-indent-first-initialiser-t.js, since that piece of implementation is
> relatively trivial), but I guess that's still much better than no tests.
>
> Regarding the meat of the change, it should be "initializer" (Emacs
> prefers US spelling), and maybe we should also extract the added lines into
> a utility function, in order not to make `js--proper-indentation' even
> longer than it is now.
>

[-- Attachment #2: Type: text/html, Size: 2908 bytes --]

Re: Requesting patch review

[Stefan Monnier]

> (Dunno who has priority here, Emacs or the spec, but I'm fine with changing
> it.)

Consistency is of no importance here, so just pick the one you like.

[ Some anal retentive contributor (most of us are, it seems) may feel
  the urge to "fix it" later on, of course, but that could happen in
  either direction anyway.  ]


        Stefan

Re: Requesting patch review

[Dmitry Gutov]

On 03/10/2015 03:34 AM, Jackson Hamilton wrote:

> (Dunno who has priority here, Emacs or the spec, but I'm fine with
> changing it.)

Up to you. Since Stefan made a point against being anal retentive, I'm 
going to shut up. :)

>>maybe we should also extract the added lines into a utility function
>
> How does `js--maybe-goto-declaration-keyword-end` sound?

Good.

Re: Requesting patch review

[Paul Eggert]

Jackson Hamilton wrote:
> (Dunno who has priority here, Emacs or the spec, but I'm fine with changing
> it.)

It's no big deal, but the Emacs source uses American spelling so I checked in a 
patch to do that as master commit 2a1be9eb2361f705e61839b78e8bba1d841944f6. 
This patch changes "-initialiser" to "-init" in the code, so that we needn't 
worry about American vs British spelling here.