Re: Requesting patch review

[Jackson Hamilton <jackson@jacksonrayhamilton.com>]

[-- 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