GNU bug report logs - #6806
Set comment-multi-line in js-mode

Previous Next

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

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

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

From: Nathan Weizenbaum <nweiz <at> google.com>
To: submit <at> debbugs.gnu.org
Subject: Set comment-multi-line in js-mode
Date: Thu, 5 Aug 2010 11:36:41 -0700

[Message part 1 (text/plain, inline)]

Package: emacs
Version: 24.0.50.1

js-mode doesn't set the comment-multi-line variable. This results in
comment-indent-new-line behaving improperly when used in a multi-line
comment, which also affects auto-fill-mode, according to the documentation
for comment-multi-line.

Attached is a patch that sets the variable.

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

[0001-Set-comment-multi-line-in-js-mode.patch (text/x-patch, attachment)]

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

From: Chong Yidong <cyd <at> stupidchicken.com>
To: Nathan Weizenbaum <nweiz <at> google.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: Set comment-multi-line in js-mode
Date: Sun, 08 Aug 2010 16:40:05 -0400

Nathan Weizenbaum <nweiz <at> google.com> writes:

> js-mode doesn't set the comment-multi-line variable. This results in
> comment-indent-new-line behaving improperly when used in a multi-line comment,
> which also affects auto-fill-mode, according to the documentation for
> comment-multi-line.
>
> Attached is a patch that sets the variable.

Could you provide a precise recipe for the problem?

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

From: Nathan Weizenbaum <nweiz <at> google.com>
To: Chong Yidong <cyd <at> stupidchicken.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: Set comment-multi-line in js-mode
Date: Sun, 8 Aug 2010 17:14:12 -0700

[Message part 1 (text/plain, inline)]

Open a new Javascript file. Type "/*". Run M-x comment-indent-new-line. This
inserts "\n  /*", which is incorrect.

Now that I try to reproduce my fix, though, it doesn't seem to work. I'm not
sure what the proper solution is.

On Sun, Aug 8, 2010 at 1:40 PM, Chong Yidong <cyd <at> stupidchicken.com> wrote:

> Nathan Weizenbaum <nweiz <at> google.com> writes:
>
> > js-mode doesn't set the comment-multi-line variable. This results in
> > comment-indent-new-line behaving improperly when used in a multi-line
> comment,
> > which also affects auto-fill-mode, according to the documentation for
> > comment-multi-line.
> >
> > Attached is a patch that sets the variable.
>
> Could you provide a precise recipe for the problem?
>

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Nathan Weizenbaum <nweiz <at> google.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: Set comment-multi-line in js-mode
Date: Mon, 09 Aug 2010 13:33:23 +0200

> js-mode doesn't set the comment-multi-line variable. This results in
> comment-indent-new-line behaving improperly when used in a multi-line
> comment, which also affects auto-fill-mode, according to the documentation
> for comment-multi-line.

If I understand and remember correctly, this is not the right fix for
your problem: setting comment-multi-line to t will fix your problem with
"/*..*/" but will introduce another for "//....\n" and vice-versa.


        Stefan

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

From: Nathan Weizenbaum <nweiz <at> google.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: Set comment-multi-line in js-mode
Date: Mon, 9 Aug 2010 10:56:56 -0700

[Message part 1 (text/plain, inline)]

My fix doesn't actually fix the "/* */" issue, unfortunately. However, it
doesn't break "//" either; note that comment-multi-line is t for e.g.
c-mode, and comment-indent-new-line works for "//" there.

On Mon, Aug 9, 2010 at 4:33 AM, Stefan Monnier <monnier <at> iro.umontreal.ca>wrote:

> > js-mode doesn't set the comment-multi-line variable. This results in
> > comment-indent-new-line behaving improperly when used in a multi-line
> > comment, which also affects auto-fill-mode, according to the
> documentation
> > for comment-multi-line.
>
> If I understand and remember correctly, this is not the right fix for
> your problem: setting comment-multi-line to t will fix your problem with
> "/*..*/" but will introduce another for "//....\n" and vice-versa.
>
>
>        Stefan
>

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Nathan Weizenbaum <nweiz <at> google.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: Set comment-multi-line in js-mode
Date: Sat, 11 Sep 2010 15:54:37 +0200

> My fix doesn't actually fix the "/* */" issue, unfortunately. However, it
> doesn't break "//" either; note that comment-multi-line is t for e.g.
> c-mode, and comment-indent-new-line works for "//" there.

Indeed, I misremembered.  Feel free to install this patch.


        Stefan
        

> On Mon, Aug 9, 2010 at 4:33 AM, Stefan Monnier <monnier <at> iro.umontreal.ca>wrote:

>> > js-mode doesn't set the comment-multi-line variable. This results in
>> > comment-indent-new-line behaving improperly when used in a multi-line
>> > comment, which also affects auto-fill-mode, according to the
>> documentation
>> > for comment-multi-line.
>> 
>> If I understand and remember correctly, this is not the right fix for
>> your problem: setting comment-multi-line to t will fix your problem with
>> "/*..*/" but will introduce another for "//....\n" and vice-versa.
>> 
>> 
>> Stefan
>> 

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

From: Chong Yidong <cyd <at> stupidchicken.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Nathan Weizenbaum <nweiz <at> google.com>, 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: Set comment-multi-line in js-mode
Date: Tue, 28 Sep 2010 14:11:02 -0400

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> My fix doesn't actually fix the "/* */" issue, unfortunately. However, it
>> doesn't break "//" either; note that comment-multi-line is t for e.g.
>> c-mode, and comment-indent-new-line works for "//" there.
>
> Indeed, I misremembered.  Feel free to install this patch.

The patch doesn't do the right thing.  The reported problem is that if
you enter "/*" in a js-mode buffer and do M-x comment-indent-new-line,
Emacs inserts another "/*".

This problem is not limited to js-mode.  It afflicts C++ also.  Try
this:

  C-x C-f foo.cc RET
  /*
  M-x comment-indent-new-line RET

Emacs inserts another /*.  The reason is this stretch of code in
newcomment.el:1311:

     (normalp
      (string-match (regexp-quote (comment-string-strip
                                   comment-start t t))
                    comstart))
     (comment-end
      (if normalp comment-end
        ;; The comment starter is not the normal comment-start
        ;; so we can't just use comment-end.
        (save-excursion
          (goto-char compos)
          (if (not (comment-forward)) comment-end
            (comment-string-strip
             (buffer-substring
              (save-excursion (comment-enter-backward) (point))
              (point))
             nil t)))))

When the default comment-start is "//" but the current comment begins in
"/*", this code tries to find the appropriate comment-end by doing
comment-forward.  But if the comment-end "*/" is not already present in
the buffer, it fails.

Any suggestion?

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

From: Tom Tromey <tom <at> tromey.com>
To: 6806 <at> debbugs.gnu.org
Subject: another reason for the patch
Date: Tue, 17 Jan 2017 21:56:18 -0700

[Message part 1 (text/plain, inline)]

Hi.  I'd like to resurrect this bug.

While working in JS I found a bug, which I tracked down to the same
change indicated here.  Basically, auto-fill in JS mode will sometimes
insert a "/*" on a filled line, where it should not.

You can see a precise recipe in the new test case, in the attached
patch.

One funny thing is that if you do this interactively (in my real-world
case I had auto-fill-mode enabled), you have to be sure that there isn't
a newline after point.  If there is a newline, the filling works
properly.

Anyway, let me know what you think.  I'd like to check this in, but,
given the comments in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6806#23
I suppose I would not close the bug.

Tom

[P (text/x-patch, inline)]

commit 5a615aeae34a257a1879134a883c61bc42f41691
Author: Tom Tromey <tom <at> tromey.com>
Date:   Tue Jan 17 21:50:14 2017 -0700

    Set comment-multi-line in js-mode
    
    Bug#6806:
    * lisp/progmodes/js.el (js-mode): Set comment-multi-line to t.
    * test/lisp/progmodes/js-tests.el (js-mode-auto-fill): New test.

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 2e5c6ae..cfd58d2 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3852,6 +3852,7 @@ js-mode
         comment-start-skip "\\(//+\\|/\\*+\\)\\s *")
   (setq-local comment-line-break-function #'c-indent-new-comment-line)
   (setq-local c-block-comment-start-regexp "/\\*")
+  (setq-local comment-multi-line t)
 
   (setq-local electric-indent-chars
 	      (append "{}():;," electric-indent-chars)) ;FIXME: js2-mode adds "[]*".
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index 84749ef..7cb737c 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -85,6 +85,20 @@
       (should (= (current-column) x))
       (forward-line))))
 
+(ert-deftest js-mode-auto-fill ()
+  (with-temp-buffer
+    (js-mode)
+    (setq fill-column 70)
+    (insert "/* ")
+    (dotimes (_ 16)
+      (insert "test "))
+    (do-auto-fill)
+    ;; The bug is that, after auto-fill, the second line starts with
+    ;; "/*", whereas it should start with " * ".
+    (goto-char (point-min))
+    (forward-line)
+    (should (looking-at " \\* test"))))
+
 (provide 'js-tests)
 
 ;;; js-tests.el ends here

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

From: Tom Tromey <tom <at> tromey.com>
To: 6806 <at> debbugs.gnu.org
Subject: checked in the patch
Date: Sat, 04 Feb 2017 13:29:11 -0700

I went ahead and checked in the patch, mostly because comment #20 said
it was fine, and also because it fixed a bug.

I think this bug should be left open, though, due to the other issues
identified.

Tom

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

From: npostavs <at> users.sourceforge.net
To: Tom Tromey <tom <at> tromey.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: another reason for the patch
Date: Sat, 04 Feb 2017 16:21:41 -0500

Tom Tromey <tom <at> tromey.com> writes:

> Hi.  I'd like to resurrect this bug.
>
> While working in JS I found a bug, which I tracked down to the same
> change indicated here.  Basically, auto-fill in JS mode will sometimes
> insert a "/*" on a filled line, where it should not.
>
> You can see a precise recipe in the new test case, in the attached
> patch.
>
> One funny thing is that if you do this interactively (in my real-world
> case I had auto-fill-mode enabled), you have to be sure that there isn't
> a newline after point.  If there is a newline, the filling works
> properly.
>
> Anyway, let me know what you think.  I'd like to check this in, but,
> given the comments in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6806#23
> I suppose I would not close the bug.

Sorry I missed this before you pushed the patch, but I'm not sure this
is a bug.  If I revert the setting of `comment-multi-line' then with
your test scenario, the result of auto fill is

    /* test test test test test test test test test test test test test */
    /* test test test 

So the opened comment was correctly terminated, and a new one started,
just as documented.  Perhaps you don't like that `comment-multi-line' is
nil by default, but it doesn't look like a bug to me.

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

From: npostavs <at> users.sourceforge.net
To: Tom Tromey <tom <at> tromey.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: another reason for the patch
Date: Sat, 04 Feb 2017 16:34:06 -0500

found 6806 24.5
fixed 6806 25.1
quit

npostavs <at> users.sourceforge.net writes:
>
>  If I revert the setting of `comment-multi-line' then with
> your test scenario, the result of auto fill is
>
>     /* test test test test test test test test test test test test test */
>     /* test test test 

Up until 24.5 the result is

    /* test test test test test test test test test test test test test
       /* test test test 

So this bug has been fixed in 25.1.

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

From: Tom Tromey <tom <at> tromey.com>
To: npostavs <at> users.sourceforge.net
Cc: 6806 <at> debbugs.gnu.org, Tom Tromey <tom <at> tromey.com>
Subject: Re: bug#6806: another reason for the patch
Date: Sat, 11 Feb 2017 08:19:04 -0700

Noam> So the opened comment was correctly terminated, and a new one started,
Noam> just as documented.  Perhaps you don't like that `comment-multi-line' is
Noam> nil by default, but it doesn't look like a bug to me.

I'm not sure what to do about this now.
Should I revert the patch?

If it were solely up to me I would leave the patch in, since I think
non-nil comment-multi-line is more normal in JS code.  And, if the patch
is reverted then I think comment-multi-line needs a :safe setting so it
can by set in .dir-locals.el.

Tom

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

From: npostavs <at> users.sourceforge.net
To: Tom Tromey <tom <at> tromey.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: another reason for the patch
Date: Sat, 11 Feb 2017 23:21:02 -0500

[Message part 1 (text/plain, inline)]

Tom Tromey <tom <at> tromey.com> writes:

> Noam> So the opened comment was correctly terminated, and a new one started,
> Noam> just as documented.  Perhaps you don't like that `comment-multi-line' is
> Noam> nil by default, but it doesn't look like a bug to me.
>
> I'm not sure what to do about this now.
> Should I revert the patch?
>
> If it were solely up to me I would leave the patch in, since I think
> non-nil comment-multi-line is more normal in JS code.  And, if the patch
> is reverted then I think comment-multi-line needs a :safe setting so it
> can by set in .dir-locals.el.

I don't really have much of an opinion regarding the best value for
comment-multi-line.  But we should test that filling works correctly
regardless of its value, and we should add a :safe setting anyway.  Here
is a patch:

[0001-Test-comment-multi-line-nil-auto-fill-case-too.patch (text/x-diff, inline)]

From cbf522f2446035bcd34a676c2ef3e641b4f20f90 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Sat, 11 Feb 2017 23:15:13 -0500
Subject: [PATCH] Test comment-multi-line = nil auto fill case too

* test/lisp/progmodes/js-tests.el (js-mode-auto-fill): Test with
`comment-multi-line' both nil and non-nil.
* lisp/newcomment.el (comment-multi-line): Mark safe if it's a
boolean.
---
 lisp/newcomment.el              |  1 +
 test/lisp/progmodes/js-tests.el | 22 ++++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index 1af89293b6..4b261c34c6 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -309,6 +309,7 @@ comment-multi-line
 It also affects \\[indent-new-comment-line].  However, if you want this
 behavior for explicit filling, you might as well use \\[newline-and-indent]."
   :type 'boolean
+  :safe #'booleanp
   :group 'comment)
 
 (defcustom comment-empty-lines nil
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index d61f084e0d..99f5898525 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -89,16 +89,18 @@
 (ert-deftest js-mode-auto-fill ()
   (with-temp-buffer
     (js-mode)
-    (setq fill-column 70)
-    (insert "/* ")
-    (dotimes (_ 16)
-      (insert "test "))
-    (do-auto-fill)
-    ;; The bug is that, after auto-fill, the second line starts with
-    ;; "/*", whereas it should start with " * ".
-    (goto-char (point-min))
-    (forward-line)
-    (should (looking-at " \\* test"))))
+    (let ((fill-column 10)
+          (comment-multi-line t))
+      (insert "/* test test")
+      (do-auto-fill)
+      ;; Filling should continue the multi line comment.
+      (should (equal (buffer-string) "/* test\n * test"))
+      (erase-buffer)
+      (insert "/* test test")
+      (setq comment-multi-line nil)
+      (do-auto-fill)
+      ;; Filling should start a new comment on the next line.
+      (should (equal (buffer-string) "/* test */\n/* test")))))
 
 (ert-deftest js-mode-regexp-syntax-bug-25529 ()
   (dolist (regexp-contents '("[^[]"
-- 
2.11.1

[Message part 3 (text/plain, inline)]

Perhaps we should also revert the js-mode comment-multi-line setting,
not sure.

--- i/lisp/progmodes/js.el
+++ w/lisp/progmodes/js.el
@@ -3856,7 +3856,6 @@ js-mode
         comment-start-skip "\\(//+\\|/\\*+\\)\\s *")
   (setq-local comment-line-break-function #'c-indent-new-comment-line)
   (setq-local c-block-comment-start-regexp "/\\*")
-  (setq-local comment-multi-line t)
 
   (setq-local electric-indent-chars
 	      (append "{}():;," electric-indent-chars)) ;FIXME: js2-mode adds "[]*".

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

From: Tom Tromey <tom <at> tromey.com>
To: npostavs <at> users.sourceforge.net
Cc: 6806 <at> debbugs.gnu.org, Tom Tromey <tom <at> tromey.com>
Subject: Re: bug#6806: another reason for the patch
Date: Sat, 11 Feb 2017 23:16:19 -0700

>>>>> "Noam" == Noam Postavsky <npostavs <at> users.sourceforge.net> writes:

Noam> Perhaps we should also revert the js-mode comment-multi-line setting,
Noam> not sure.

My belief is that more JS code follows the comment-multi-line style than
follows the other style.

Tom

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

From: npostavs <at> users.sourceforge.net
To: Tom Tromey <tom <at> tromey.com>
Cc: 6806 <at> debbugs.gnu.org
Subject: Re: bug#6806: another reason for the patch
Date: Tue, 14 Feb 2017 23:50:10 -0500

close 6806
quit

Tom Tromey <tom <at> tromey.com> writes:

>>>>>> "Noam" == Noam Postavsky <npostavs <at> users.sourceforge.net> writes:
>
> Noam> Perhaps we should also revert the js-mode comment-multi-line setting,
> Noam> not sure.
>
> My belief is that more JS code follows the comment-multi-line style than
> follows the other style.

Okay, I pushed the patch without changing comment-multi-line back [1:
0a64666288]; I added a mention about the new comment-multi-line setting
in NEWS, we'll see if anyone objects.

1: 2017-02-14 22:29:56 -0500 0a64666288e3f32967db4ad683a4bc2f225fb952
  Test comment-multi-line = nil auto fill case too

Previous Next

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