GNU bug report logs - #25904
Formatting bug with js-mode

Previous Next

Package: emacs;

Reported by: Michael Snead <aikeru <at> gmail.com>

Date: Tue, 28 Feb 2017 23:07:02 UTC

Severity: minor

Fixed in version 27.1

Done: Dmitry Gutov <dgutov <at> yandex.ru>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 25904 in the body.
You can then email your comments to 25904 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Tue, 28 Feb 2017 23:07:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Michael Snead <aikeru <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Tue, 28 Feb 2017 23:07:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Michael Snead <aikeru <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Formatting bug with js-mode
Date: Tue, 28 Feb 2017 17:50:19 -0500
[Message part 1 (text/plain, inline)]
Hi there. Apologies if this is the wrong place to submit a bug on js-mode
for emacs. I am an emacs newbie (in fact, I've only used spacemacs and
still relatively new there too) and not sure where to post about this.

Basically I am trying to find the right place to report this issue, which
was determined to be an upstream issue with js-mode:

https://github.com/felipeochoa/rjsx-mode/issues/34

In essence, a fat arrow expression followed by a linefeed, followed by an
expression (such as a JSX tag with children) that is not wrapped in
parenthesis is valid syntax and recognized by editors like WebStorm and
VSCode but doesn't work correctly in this editor.

If this is not the right place I would be grateful for directions on where
this bug belongs.

Thanks!
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Tue, 07 Nov 2017 23:56:01 GMT) Full text and rfc822 format available.

Message #8 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Felipe Ochoa <felipeochoa0918 <at> gmail.com>
To: 25904 <at> debbugs.gnu.org
Subject: Re: Formatting bug with js-mode
Date: Tue, 07 Nov 2017 18:55:11 -0500
Here's an initial implementation of a solution for this 
issue. (Also filed as js2#314 [1])

foo.bar.baz(very => 
 very  // current code aligns return value to paren above 
).biz(baz => 
 baz
);

I've been using this code successfully for a number of days now, 
but since it's my first time touching indentation code, it likely 
needs review for I comments and whitespace handling, etc.

(Note: disabling `js-indent-align-list-continuation' also 
addresses this issue, but disabling it changes indentation in 
other parts.)

[1]: https://github.com/mooz/js2-mode/issues/314

---
lisp/progmodes/js.el | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 1f86909..18f888d 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1848,7 +1848,9 @@ This performs fontification according to `js--class-styles'."
             (skip-chars-backward " \t")
             (or (bobp) (backward-char))
             (and (> (point) (point-min))
-                  (save-excursion (backward-char) (not (looking-at "[/*]/")))
+                  ;; Need to exclude => here since js--looking-at-operator-p thinks
+                  ;; it's looking at an assignment operator
+                  (save-excursion (backward-char) (not (looking-at "[/*]/\\|=>")))
                  (js--looking-at-operator-p)
                  (and (progn (backward-char)
                              (not (looking-at "+\\+\\|--\\|/[/*]"))))))))))
@@ -2107,7 +2109,18 @@ indentation is aligned to that column."
                 (continued-expr-p (js--continued-expression-p)))
             (goto-char (nth 1 parse-status)) ; go to the opening char
             (if (or (not js-indent-align-list-continuation)
-                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
+                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
+                     ;; check for a arrow function without parens
+                     (and (looking-at "(\\s-*\\(async\\s-*\\)?")
+                          ;; TODO: should call (forward-comment most-positive-fixnum)?
+                          (save-excursion
+                            (goto-char (match-end 0))
+                            (cond
+                             ((eq (char-after) ?\()
+                              (forward-sexp)
+                              (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
+                             (t (looking-at-p
+                                 (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))
                 (progn ; nothing following the opening paren/bracket
                   (skip-syntax-backward " ")
                   (when (eq (char-before) ?\)) (backward-list))
--
2.7.4




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Sun, 12 Nov 2017 23:03:02 GMT) Full text and rfc822 format available.

Message #11 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Felipe Ochoa <felipeochoa0918 <at> gmail.com>, 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Formatting bug with js-mode
Date: Mon, 13 Nov 2017 01:02:28 +0200
Hi Felipe!

On 11/8/17 1:55 AM, Felipe Ochoa wrote:
> Here's an initial implementation of a solution for this issue. (Also 
> filed as js2#314 [1])
> 
> foo.bar.baz(very =>  very  // current code aligns return value to paren 
> above ).biz(baz =>  baz
> );
> 
> I've been using this code successfully for a number of days now, but 
> since it's my first time touching indentation code, it likely needs 
> review for I comments and whitespace handling, etc.

I think the code is reasonable, but the patch needs a few examples for 
emacs/test/manual/js*.js. Maybe add a new file, or maybe reuse an 
existing one.

It also wouldn't hurt that the existing examples are unchanged with the 
new code.

>               (skip-chars-backward " \t")
>               (or (bobp) (backward-char))
>               (and (> (point) (point-min))
> -                  (save-excursion (backward-char) (not (looking-at 
> "[/*]/")))
> +                  ;; Need to exclude => here since 
> js--looking-at-operator-p thinks
> +                  ;; it's looking at an assignment operator
> +                  (save-excursion (backward-char) (not (looking-at 
> "[/*]/\\|=>")))

OK.

> @@ -2107,7 +2109,18 @@ indentation is aligned to that column."
>                   (continued-expr-p (js--continued-expression-p)))
>               (goto-char (nth 1 parse-status)) ; go to the opening char
>               (if (or (not js-indent-align-list-continuation)
> -                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
> +                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
> +                     ;; check for a arrow function without parens
> +                     (and (looking-at "(\\s-*\\(async\\s-*\\)?")
> +                          ;; TODO: should call (forward-comment 
> most-positive-fixnum)?

To allow comments between the opening paren and the arglist? Does 
anybody write them there?

> +                          (save-excursion
> +                            (goto-char (match-end 0))
> +                            (cond
> +                             ((eq (char-after) ?\()
> +                              (forward-sexp)
> +                              (looking-at-p 
> "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
> +                             (t (looking-at-p
> +                                 (concat js--name-re 
> "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))

I imagine this (*) could be transformed into a single regexp, though it 
would be pretty complex (rx could help, though!).

(*) Looking at an opening paren followed by ([optional] lambda arglist 
and an arrow) then [optional] comment and newline.

Arglist matcher might be on the big side, but I'm guessing the 
performance will be better. Not sure how big of an issue this is.

If it's not one regexp, moving some of the new code into a helper 
function (with a sensible name) seems prudent.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Mon, 20 Nov 2017 22:23:01 GMT) Full text and rfc822 format available.

Message #14 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Felipe Ochoa <felipeochoa0918 <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Formatting bug with js-mode
Date: Mon, 20 Nov 2017 17:22:14 -0500
Hi Dmitry!

> I think the code is reasonable, but the patch needs a few 
> examples for emacs/test/manual/js*.js. Maybe add a new file, or 
> maybe reuse an existing one.

I've added a test to js.js in the latest patch below. 

> It also wouldn't hurt that the existing examples are unchanged 
> with the new code.

I checked all the existing examples manually and all were 
unchanged when applying indent-region on the entire buffer.

> To allow comments between the opening paren and the arglist? 
> Does anybody write them there?

Oops -- this comment was supposed to be after the arrow. I was 
thinking of return type annotation comments, but I just checked 
flow and I don't think they support this. We can just remove the 
comment

> I imagine this (*) could be transformed into a single regexp, 
> though it would be pretty complex (rx could help, though!).
>
> (*) Looking at an opening paren followed by ([optional] lambda 
> arglist and an arrow) then [optional] comment and newline.

Is there an arglist regexp already in use somewhere? I'd rather 
not roll my own since dealing with default argument values and 
spreads and such seems quite challenging.

> If it's not one regexp, moving some of the new code into a 
> helper function (with a sensible name) seems prudent.

I've done this in the latest patch.

---
lisp/progmodes/js.el     | 26 ++++++++++++++++++++++++--
test/manual/indent/js.js |  9 +++++++++
2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 1f86909..6e057b0 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1848,7 +1848,9 @@ This performs fontification according to `js--class-styles'."
             (skip-chars-backward " \t")
             (or (bobp) (backward-char))
             (and (> (point) (point-min))
-                  (save-excursion (backward-char) (not (looking-at "[/*]/")))
+                  ;; Need to exclude => here since js--looking-at-operator-p thinks
+                  ;; it's looking at an assignment operator
+                  (save-excursion (backward-char) (not (looking-at "[/*]/\\|=>")))
                  (js--looking-at-operator-p)
                  (and (progn (backward-char)
                              (not (looking-at "+\\+\\|--\\|/[/*]"))))))))))
@@ -2077,6 +2079,21 @@ indentation is aligned to that column."
        (when comma-p
          (goto-char (1+ declaration-keyword-end))))))))

+(defun js--looking-at-broken-arrow-function-p ()
+  "Helper function for `js--proper-indentation'.
+Return t if point is at the start of a (possibly async) arrow
+function and the last non-comment, non-whitespace token of the
+current like is the \"=>\" token."
+  (and (looking-at "\\s-*\\(async\\s-*\\)?")
+       (save-excursion
+         (goto-char (match-end 0))
+         (cond
+          ((eq (char-after) ?\()
+           (forward-list) ; Is this better than forward-sexp?
+           (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
+          (t (looking-at-p
+              (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))
+
(defun js--proper-indentation (parse-status)
  "Return the proper indentation for the current line."
  (save-excursion
@@ -2107,7 +2124,12 @@ indentation is aligned to that column."
                 (continued-expr-p (js--continued-expression-p)))
             (goto-char (nth 1 parse-status)) ; go to the opening char
             (if (or (not js-indent-align-list-continuation)
-                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
+                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
+                     (when (eq (char-after) ?\()
+                       (save-excursion
+                         (forward-char)
+                         (skip-syntax-forward " ")
+                         (js--looking-at-broken-arrow-function-p))))
                 (progn ; nothing following the opening paren/bracket
                   (skip-syntax-backward " ")
                   (when (eq (char-before) ?\)) (backward-list))
diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
index b0d8bca..2286cc1 100644
--- a/test/manual/indent/js.js
+++ b/test/manual/indent/js.js
@@ -144,6 +144,15 @@ bar(
  /abc/
)

+// #25904
+foo.bar.baz(very =>
+  very
+).biz(([baz={a: [123]}, boz]) =>
+  baz
+).snarf((snorf) =>
+  snorf
+);
+
// Local Variables:
// indent-tabs-mode: nil
// js-indent-level: 2
--
2.7.4




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Thu, 23 Nov 2017 21:42:02 GMT) Full text and rfc822 format available.

Message #17 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Felipe Ochoa <felipeochoa0918 <at> gmail.com>
Cc: 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Formatting bug with js-mode
Date: Thu, 23 Nov 2017 23:41:45 +0200
On 11/21/17 12:22 AM, Felipe Ochoa wrote:

> I've added a test to js.js in the latest patch below.

Thanks.

>> To allow comments between the opening paren and the arglist? Does 
>> anybody write them there?
> 
> Oops -- this comment was supposed to be after the arrow. I was thinking 
> of return type annotation comments, but I just checked flow and I don't 
> think they support this. We can just remove the comment

Sure.

> Is there an arglist regexp already in use somewhere? I'd rather not roll 
> my own since dealing with default argument values and spreads and such 
> seems quite challenging.

No, sorry, I forgot about destructuring and etc. forward-list is fine here.

>> If it's not one regexp, moving some of the new code into a helper 
>> function (with a sensible name) seems prudent.
> 
> I've done this in the latest patch.

Thanks.

> +(defun js--looking-at-broken-arrow-function-p ()
> +  "Helper function for `js--proper-indentation'.
> +Return t if point is at the start of a (possibly async) arrow
> +function and the last non-comment, non-whitespace token of the
> +current like is the \"=>\" token."
> +  (and (looking-at "\\s-*\\(async\\s-*\\)?")

This looks weird: the regexp will always match. Better to write it as

(if (looking-at NON-OPT-REGEXP) (goto-char ...)), where NON-OPT-REGEXP 
does not include the optional qualifier (?).

And the (save-excursion...) form seems unnecessary, since the caller 
already uses it.

> +       (save-excursion
> +         (goto-char (match-end 0))
> +         (cond
> +          ((eq (char-after) ?\()
> +           (forward-list) ; Is this better than forward-sexp?

I think so: it should be a bit faster, and it doesn't require you to 
bind forward-sexp-function to nil (for e.g. js2-mode).

> +           (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
> +          (t (looking-at-p
> +              (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))

Should we extract "\\s-*=>\\s-*\\(/[/*]\\|$\\)" to a local variable, or 
a constant?

> (defun js--proper-indentation (parse-status)
>    "Return the proper indentation for the current line."
>    (save-excursion
> @@ -2107,7 +2124,12 @@ indentation is aligned to that column."
>                   (continued-expr-p (js--continued-expression-p)))
>               (goto-char (nth 1 parse-status)) ; go to the opening char
>               (if (or (not js-indent-align-list-continuation)
> -                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
> +                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
> +                     (when (eq (char-after) ?\()
> +                       (save-excursion
> +                         (forward-char)
> +                         (skip-syntax-forward " ")

This seems a bit extraneous: the regexps in the called function all 
start with "\\s-*", right? Let's maybe have the one or the other.

> +                         (js--looking-at-broken-arrow-function-p))))
>                   (progn ; nothing following the opening paren/bracket
>                     (skip-syntax-backward " ")
>                     (when (eq (char-before) ?\)) (backward-list))
> diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
> index b0d8bca..2286cc1 100644
> --- a/test/manual/indent/js.js
> +++ b/test/manual/indent/js.js
> @@ -144,6 +144,15 @@ bar(
>    /abc/
> )
> 
> +// #25904

We write references to debbugs as bug#25904. bug-reference-mode can 
linkify the result.

> +foo.bar.baz(very =>
> +  very
> +).biz(([baz={a: [123]}, boz]) =>
> +  baz
> +).snarf((snorf) =>
> +  snorf
> +);
> +

Thank you.

Please attach the resulting patch as a file produced by 'git 
format-patch', with a ChangeLog style message entry, as described in 
CONTRIBUTE.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Fri, 16 Mar 2018 01:08:02 GMT) Full text and rfc822 format available.

Message #20 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Felipe Ochoa <felipe.nospam.ochoa <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Formatting bug with js-mode
Date: Thu, 15 Mar 2018 20:06:57 -0500
This has taken a *little* longer than I'd hoped, but here's a new
patch with all the discussed changes. Hopefully I got the commit
message formatting and content right

From 0c7eed3fd9f7f77a071296c191e8569471bac4e5 Mon Sep 17 00:00:00 2001
From: Felipe Ochoa <felipe.nospam.ochoa <at> gmail.com>
Date: Tue, 7 Nov 2017 16:50:42 -0500
Subject: [PATCH] Fixes js indentation after arrow function without parens

From: felipe <felipe <at> fov.space>

* lisp/progmodes/js.el (js--proper-indentation)
(js--looking-at-broken-arrow-function-p)
* test/manual/indent/js.js: Indent expression following an arrow as
though it were enclosed in parentheses

(Bug#25904)
---
 lisp/progmodes/js.el     | 24 ++++++++++++++++++++++--
 test/manual/indent/js.js |  9 +++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index f30e591..5a333a9 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1849,7 +1849,9 @@ js--continued-expression-p
              (skip-chars-backward " \t")
              (or (bobp) (backward-char))
              (and (> (point) (point-min))
-                  (save-excursion (backward-char) (not (looking-at "[/*]/")))
+                  ;; Need to exclude => here since
js--looking-at-operator-p thinks
+                  ;; it's looking at an assignment operator
+                  (save-excursion (backward-char) (not (looking-at
"[/*]/\\|=>")))
                   (js--looking-at-operator-p)
                   (and (progn (backward-char)
                               (not (looking-at "+\\+\\|--\\|/[/*]"))))))))))
@@ -2078,6 +2080,23 @@ js--maybe-goto-declaration-keyword-end
         (when comma-p
           (goto-char (1+ declaration-keyword-end))))))))

+(defconst js--line-terminating-arrow "\\s-*=>\\s-*\\(/[/*]\\|$\\)"
+  "Regexp to match a \"=>\" token if it's the last non-whitespace,
non-comment token in a line.")
+
+(defun js--looking-at-broken-arrow-function-p ()
+  "Helper function for `js--proper-indentation'.
+Return t if point is at the start of a (possibly async) arrow
+function and the last non-comment, non-whitespace token of the
+current like is the \"=>\" token."
+  (when (looking-at "\\s-*async\\s-*")
+    (goto-char (match-end 0)))
+  (cond
+   ((eq (char-after) ?\()
+    (forward-list)
+    (looking-at-p js--line-terminating-arrow))
+   (t (looking-at-p
+       (concat js--name-re js--line-terminating-arrow)))))
+
 (defun js--proper-indentation (parse-status)
   "Return the proper indentation for the current line."
   (save-excursion
@@ -2108,7 +2127,8 @@ js--proper-indentation
                  (continued-expr-p (js--continued-expression-p)))
              (goto-char (nth 1 parse-status)) ; go to the opening char
              (if (or (not js-indent-align-list-continuation)
-                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
+                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
+                     (progn (forward-char)
(js--looking-at-broken-arrow-function-p)))
                  (progn ; nothing following the opening paren/bracket
                    (skip-syntax-backward " ")
                    (when (eq (char-before) ?\)) (backward-list))
diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
index b0d8bca..fc39c12 100644
--- a/test/manual/indent/js.js
+++ b/test/manual/indent/js.js
@@ -144,6 +144,15 @@ bar(
   /abc/
 )

+// bug#25904
+foo.bar.baz(very => // A comment
+  very
+).biz(([baz={a: [123]}, boz]) =>
+  baz
+).snarf((snorf) => /* Another comment */
+  snorf
+);
+
 // Local Variables:
 // indent-tabs-mode: nil
 // js-indent-level: 2
-- 
2.7.4




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Sun, 29 Apr 2018 01:21:01 GMT) Full text and rfc822 format available.

Message #23 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
To: 25904 <at> debbugs.gnu.org
Cc: Felipe Ochoa <felipe.nospam.ochoa <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#24896: JSX prop indentation after fat arrow
Date: Sun, 29 Apr 2018 02:20:39 +0100
If this patch works, can we make it into Emacs 26 before the release?

Thanks a lot for working on this. This has been the most annoying bug in
js-mode.






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Sun, 29 Apr 2018 02:44:01 GMT) Full text and rfc822 format available.

Message #26 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
Cc: 25904 <at> debbugs.gnu.org, felipe.nospam.ochoa <at> gmail.com, dgutov <at> yandex.ru
Subject: Re: bug#25904: bug#24896: JSX prop indentation after fat arrow
Date: Sun, 29 Apr 2018 05:43:06 +0300
> From: Jimmy Yuen Ho Wong <wyuenho <at> gmail.com>
> Date: Sun, 29 Apr 2018 02:20:39 +0100
> Cc: Felipe Ochoa <felipe.nospam.ochoa <at> gmail.com>,
> 	Dmitry Gutov <dgutov <at> yandex.ru>
> 
> If this patch works, can we make it into Emacs 26 before the release?

No, it's too late for that.  Sorry.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Fri, 04 May 2018 21:38:01 GMT) Full text and rfc822 format available.

Message #29 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Felipe Ochoa <felipe.nospam.ochoa <at> gmail.com>
Cc: 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Formatting bug with js-mode
Date: Sat, 5 May 2018 00:37:13 +0300
On 3/16/18 3:06 AM, Felipe Ochoa wrote:
> This has taken a *little* longer than I'd hoped, but here's a new
> patch with all the discussed changes. Hopefully I got the commit
> message formatting and content right

Not really. First of all, I asked for an attached file ('git am' needs a 
file), and not its contents inside the message body. Which suffered 
word-wrap damage during transmission (because that's what email clients do).

I would commit it this time anyway, though, if not for the next point.

>  From 0c7eed3fd9f7f77a071296c191e8569471bac4e5 Mon Sep 17 00:00:00 2001
> From: Felipe Ochoa <felipe.nospam.ochoa <at> gmail.com>
> Date: Tue, 7 Nov 2017 16:50:42 -0500
> Subject: [PATCH] Fixes js indentation after arrow function without parens
> 
> From: felipe <felipe <at> fov.space>
> 
> * lisp/progmodes/js.el (js--proper-indentation)
> (js--looking-at-broken-arrow-function-p)

I think there was going to be some description here.

And there's no mention of changes to js--continued-expression-p and 
js--proper-indentation. They would have the meat of the explanation.

> * test/manual/indent/js.js: Indent expression following an arrow as
> though it were enclosed in parentheses

This needs a period, at least.

> (Bug#25904)

We usually put the bug number inside one of the sentences of the change 
log entry, not just at the end of it.

The patch does work, but if you could make the details right, it would 
be great. We've missed the emacs-26 train, so there's no real hurry.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Sun, 10 Feb 2019 22:04:01 GMT) Full text and rfc822 format available.

Message #32 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Jackson Ray Hamilton <jackson <at> jacksonrayhamilton.com>
To: 25904 <at> debbugs.gnu.org
Subject: Patches
Date: Sun, 10 Feb 2019 14:03:44 -0800 (PST)
[Message part 1 (text/plain, inline)]
Hello Dmitry and Felipe,

I’ve taken a stab at formatting Felipe’s fix for arrow function indentation according to the coding standards.  Compared to his patch, I made some trivial name/doc changes, and changed

(progn (forward-char) (js--looking-at-broken-arrow-function-p))

to

(save-excursion (forward-char) (js--looking-at-broken-arrow-function-p))

since we probably want to undo the forward-char in case the form returns nil.

In order to get all the JS indentation tests to pass with make, I also had to make a variable safe file-locally.

Both changes are attached.  They could be applied to the emacs-26 branch.  (Or master, but emacs-26 would be preferable so we can deliver it sooner.  I tested on both branches.)

Jackson
[Message part 2 (text/html, inline)]
[0001-Indent-arrows-expression-bodies-like-function-bodies.patch (text/x-patch, attachment)]
[0001-js-indent-align-list-continuation-Make-variable-safe.patch (text/x-patch, attachment)]

Reply sent to Dmitry Gutov <dgutov <at> yandex.ru>:
You have taken responsibility. (Wed, 13 Feb 2019 00:28:02 GMT) Full text and rfc822 format available.

Notification sent to Michael Snead <aikeru <at> gmail.com>:
bug acknowledged by developer. (Wed, 13 Feb 2019 00:28:02 GMT) Full text and rfc822 format available.

Message #37 received at 25904-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Jackson Ray Hamilton <jackson <at> jacksonrayhamilton.com>,
 25904-done <at> debbugs.gnu.org
Subject: Re: bug#25904: Patches
Date: Wed, 13 Feb 2019 03:27:26 +0300
Version: 27.1

On 11.02.2019 01:03, Jackson Ray Hamilton wrote:

> Both changes are attached.  They could be applied to the emacs-26 
> branch.  (Or master, but emacs-26 would be preferable so we can deliver 
> it sooner.  I tested on both branches.)

Pushed to master, thank you.

Regarding emacs-26, please feel free to lobby it with Eli. I think it's 
a fine patch, but I'm not sure it's important enough regression-wise.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Wed, 13 Feb 2019 04:02:01 GMT) Full text and rfc822 format available.

Message #40 received at 25904-done <at> debbugs.gnu.org (full text, mbox):

From: Jackson Ray Hamilton <jackson <at> jacksonrayhamilton.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>, 25904-done <at> debbugs.gnu.org
Subject: Re: bug#25904: Patches
Date: Tue, 12 Feb 2019 20:01:15 -0800 (PST)
[Message part 1 (text/plain, inline)]
Fair enough, thanks for the merge.

> On February 12, 2019 at 4:27 PM Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>
>
> Version: 27.1
>
> On 11.02.2019 01:03, Jackson Ray Hamilton wrote:
>
> > Both changes are attached.  They could be applied to the emacs-26
> > branch.  (Or master, but emacs-26 would be preferable so we can deliver
> > it sooner.  I tested on both branches.)
>
> Pushed to master, thank you.
>
> Regarding emacs-26, please feel free to lobby it with Eli. I think it's
> a fine patch, but I'm not sure it's important enough regression-wise.
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Wed, 13 Feb 2019 15:16:02 GMT) Full text and rfc822 format available.

Message #43 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: aikeru <at> gmail.com, 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Patches
Date: Wed, 13 Feb 2019 17:15:37 +0200
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Wed, 13 Feb 2019 03:27:26 +0300
> 
> Regarding emacs-26, please feel free to lobby it with Eli. I think it's 
> a fine patch, but I'm not sure it's important enough regression-wise.

The second one, the one which makes the variable safe in some cases,
can be cherry-picked to emacs-26.  I believe it's independent of the
main change, quite safe, and is a Good Thing anyway.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#25904; Package emacs. (Thu, 14 Feb 2019 01:21:02 GMT) Full text and rfc822 format available.

Message #46 received at 25904 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: aikeru <at> gmail.com, 25904 <at> debbugs.gnu.org
Subject: Re: bug#25904: Patches
Date: Thu, 14 Feb 2019 04:20:38 +0300
On 13.02.2019 18:15, Eli Zaretskii wrote:
> The second one, the one which makes the variable safe in some cases,
> can be cherry-picked to emacs-26.  I believe it's independent of the
> main change, quite safe, and is a Good Thing anyway.

OK, done.

Not what the question was about, though.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 14 Mar 2019 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 5 years and 16 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.