bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

1. User option `comment-style' should be documented in the Emacs manual.
It is in neither that manual nor the Elisp manual.

Don't bother to answer that not all options are described in the manual
- your stock reply.  This is a bug report about a _particular_ option's
(lack of) doc.

2. The doc string of `comment-style' should explain what each of the
possible values is/does.  It should not refer you to the doc string for
`comment-styles' (a defconst).

The `comment-style' doc string should explain the values in a way that
makes sense for `M-x customize-option custom-style', that is, not just
with reference to the Lisp values but in terms of the `Value Menu'
labels.

Users of Customize should not just be sent to a defconst doc string that
requires them to somehow map the `Value Menu' labels against the Lisp
values.  It is perfectly OK to _also_ say "see also `comment-style'.
And the latter's doc string _should_ of course describe the Lisp values.

3. The doc string of `comment-styles' is anyway out of date.  Not all of
the values are described.  This means that users trying to customize
`comment-style' are lost.

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2010-11-22 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4) --no-opt --cflags
-Ic:/imagesupport/include'

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Stefan Monnier]

> 1. User option `comment-style' should be documented in the Emacs manual.

Indeed.  It probably doesn't need to be in the Elisp manual, but the
User manual should mention it.

> 2. The doc string of `comment-style' should explain what each of the
> possible values is/does.  It should not refer you to the doc string for
> `comment-styles' (a defconst).

From a modularity point of view, this is somewhat problematic.
So I have philosophical problems with this, although I agree that the
current docstring doesn't work well in Custom.

Maybe a solution is to extend comment-styles so it includes a short
docstring for each entry that can then be used for the label on each
value in the "Value menu" of comment-style.

> 3. The doc string of `comment-styles' is anyway out of date.  Not all of
> the values are described.  This means that users trying to customize
> `comment-style' are lost.

I don't understand: the docstring of comment-styles (like all other
variable docstrings) doesn't describe its current value, but rather what
are its possible value(s) and what they mean.  It seems to be just as
up-to-date as when I originally wrote it (that part of the code hasn't
changed since, AFAIK).


        Stefan

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

> > 2. The doc string of `comment-style' should explain what each of the
> > possible values is/does.  It should not refer you to the 
> > doc string for `comment-styles' (a defconst).
> 
> From a modularity point of view, this is somewhat problematic.
> So I have philosophical problems with this,

Yes, I recognize that.  It's a general problem.  But some individual cases can
be improved to some extent.  But modularity is impacted, since we have two
variables.  It could be OK for one to refer to the other, but in this case one
is a defcustom (so the values should be user-recognizable) and the other is a
defconst (so the values need to be the Lisp symbols).

> although I agree that the
> current docstring doesn't work well in Custom.
> 
> Maybe a solution is to extend comment-styles so it includes a short
> docstring for each entry that can then be used for the label on each
> value in the "Value menu" of comment-style.

If that can be done so that updates work automatically, that would be good.
IOW, I think you're saying that we would update the descriptions in only one
place, and they would appear in both places.

> > 3. The doc string of `comment-styles' is anyway out of 
> > date.  Not all of the values are described.  This means that
> > users trying to customize `comment-style' are lost.
> 
> I don't understand: the docstring of comment-styles (like all other
> variable docstrings) doesn't describe its current value, but 
> rather what
> are its possible value(s) and what they mean.  It seems to be just as
> up-to-date as when I originally wrote it (that part of the code hasn't
> changed since, AFAIK).

The possible values seem to include `box' and `box-multi' (at least I see those
in the default alist value).  Neither of those is described in the doc string.

Comment region styles of the form (STYLE . (MULTI ALIGN EXTRA INDENT)).
STYLE should be a mnemonic symbol.
MULTI specifies that comments are allowed to span multiple lines.
ALIGN specifies that the `comment-end' markers should be aligned.
EXTRA specifies that an extra line should be used before and after the
  region to comment (to put the `comment-end' and `comment-start').
INDENT specifies that the `comment-start' markers should not be put at the
  left margin but at the current indentation of the region to comment.
If INDENT is `multi-char', that means indent multi-character
  comment starters, but not one-character comment starters.

Value: 
((plain nil nil nil nil)
 (indent nil nil nil t)
 (indent-or-triple nil nil nil multi-char)
 (aligned nil t nil t)
 (multi-line t nil nil t)
 (extra-line t nil t t)
 (box nil t t t) ; <=====================
 (box-multi t t t t)) ; <================

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

> > I don't understand: the docstring of comment-styles (like all other
> > variable docstrings) doesn't describe its current value, but 
> > rather what are its possible value(s) and what they mean.  It
> > seems to be just as up-to-date as when I originally wrote it
> 
> The possible values seem to include `box' and `box-multi' (at 
> least I see those in the default alist value).  Neither of those
> is described in the doc string.

I was mistaken.  I was interpreting the alist values wrt the labels in the menu
for `comment-style'.  I guess that all that's missing are descriptions of the
latter. 

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Stefan Monnier]

Can you try the patch below and tell us if it addresses your problem
(I'm not that happy with the doc for some of those styles, so if you
have suggestions for improvements, I'm all ears).


        Stefan
        
        
=== modified file 'lisp/newcomment.el'
--- lisp/newcomment.el	2010-10-22 13:45:31 +0000
+++ lisp/newcomment.el	2010-11-29 18:26:11 +0000
@@ -187,16 +187,18 @@
 it is 1 so that regions are commented with two or three semi-colons.")
 
 (defconst comment-styles
-  '((plain	. (nil nil nil nil))
-    (indent	. (nil nil nil t))
+  '((plain    nil nil nil nil "Comment in column 0"))
+    (indent   nil nil nil t "Indented as code"))
     (indent-or-triple
-                . (nil nil nil multi-char))
-    (aligned	. (nil t nil t))
-    (multi-line	. (t nil nil t))
-    (extra-line	. (t nil t t))
-    (box	. (nil t t t))
-    (box-multi	. (t t t t)))
-  "Comment region styles of the form (STYLE . (MULTI ALIGN EXTRA INDENT)).
+                nil nil nil multi-char "Indent only multi-char starters"))
+    (aligned	nil t nil t "Indented and with closers aligned"))
+    (multi-line	t nil nil t "Use multi-line comments"))
+    (extra-line	t nil t   t "Multi-line with starter&ender on their own line"))
+    (box	nil t t   t "Wrap comments in boxes"))
+    (box-multi	t   t t   t "Boxes using multi-line comments")))
+  "Comment region style definitions.
+Each style is defined with a form (STYLE . (MULTI ALIGN EXTRA INDENT DOC)).
+DOC should succinctly describe the style.
 STYLE should be a mnemonic symbol.
 MULTI specifies that comments are allowed to span multiple lines.
 ALIGN specifies that the `comment-end' markers should be aligned.
@@ -210,7 +212,8 @@
 (defcustom comment-style 'indent
   "Style to be used for `comment-region'.
 See `comment-styles' for a list of available styles."
-  :type `(choice ,@(mapcar (lambda (s) `(const ,(car s))) comment-styles))
+  :type `(choice ,@(mapcar (lambda (s) `(const :tag ,(nth 5 s) ,(car s)))
+                           comment-styles))
   :version "23.1"
   :group 'comment)
 

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

> Can you try the patch below and tell us if it addresses your problem
> (I'm not that happy with the doc for some of those styles, so if you
> have suggestions for improvements, I'm all ears).

Thanks for working on this.

I came across some variable definition weirdness (unrelated to this bug):

I edited the code in another file (a copy), to apply the patch, then used C-M-x
to redefine each var.

In Customize for `comment-style' I clicked the link for `comment-styles', and it
said the var is defined in `newcomment.el' (I evaled the new definition in a
different file).  And the doc was still the doc of the old definition.

Also, the Customize buffer for `comment-style' said that the value was changed
outside Customize (dunno if defining a defcustom should be considered that way,
but whatever...).

I clicked Restore to Standard Setting, and I got this error: (void-variable
indent).  From then on whenever I clicked the State button, instead of getting a
State menu I got this:

Debugger entered--Lisp error: (void-variable indent)
  eval(indent)
  (equal (eval (car cval)) (default-value symbol))
  (not (equal (eval (car cval)) (default-value symbol)))
  (and cval (default-boundp symbol) (not (equal (eval (car cval)) (default-value
symbol))))
  (let* ((symbol (widget-get-tag-or-value widget))...)
  (lambda (widget) (let* ((symbol (widget-get-tag-or-value widget))...)
  custom-menu-filter((("Set for Current Session" custom-variable-set ...)
  custom-variable-action((custom-variable :documentation-shown t...)
  widget-apply((custom-variable :documentation-shown t...)
  widget-parent-action((custom-magic :args (nil) :parent (custom-variable...)
  widget-apply((custom-magic :args (nil) :parent (custom-variable...)
  widget-parent-action((choice-item :help-echo "Change the state of this
item."...)
  widget-apply((choice-item :help-echo "Change the state of this item."...)
  widget-apply-action((choice-item :help-echo "Change the state of this
item."...)
  byte-code("...

I started over (emacs -Q)...  Same thing, but this time I didn't restore to
standard setting (so no error was raised).

Seems there is something buggy about the Customize code or C-M-x or both.  C-M-x
on a defcustom or a defconst in file foo.el should not give a *Help* buffer that
says that the var is defined in file bar.el, and it should not give the doc from
a definition in bar.el.

I tried (makunbound 'comment-style) and (makunbound 'comment-styles), then used
`C-M-x' again.  The `makunbound' calls worked OK (as shown by `boundp').  And
C-h v then showed the correct doc strings.  But it also still showed the wrong
file name: "comment-styles is a variable defined in `newcomment.el'".  So *Help*
shows a doc string that is not even present in the file it mentions!

What's the magic to competely undo a defcustom or defconst - `makunbound'
doesn't seem to be enough.


Anyway, back to this bug...

The problem with my commenting on the description text is that I don't know what
the various behaviors are, so I can't suggest doc improvements.  Yes, that means
that the current descriptions are inadequate, at least for me - I still have no
clue what is meant.

Can you please give me an example of each style, for a context/language
appropriate to that style?  Then I can try to suggest better one-liner
descriptions.  If the behavior is different depending on whether comment-end is
nil, then give me an example of each behavior.

Also, if a given style has no effect (or has the same effect as some other
style) when comment-end is nil then the doc should say that.  E.g., if
`box-multi' is the same as `multi-line' (just a wild guess) for a language where
there is no comment-end, then we should point this out somehow.

The only style description I really understand is this one: "Comment in column
0".  My (minor) suggestion here is to say "comment chars" or "comment
beginning", not just "comment".

In sum, for me to help here, you will need to give me more info about the
styles.  Don't try to word the descriptions; just give me a more verbose
explanation/description - or examples.  Then I'll suggest some descriptive text.

Other than the particular text to be used, the patch (design) seems good to me.

Another minor nit: "See `comment-styles' for a list..." should say "See
`comment-styles' for the list...".  It is precisely the same list as what is
available here.

HTH.

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Stefan Monnier]

> I came across some variable definition weirdness (unrelated to this
> bug):

I must say I have no idea how you got into such a mess.  My only
explanation would be that some part of the patch did not apply properly.

> Seems there is something buggy about the Customize code or C-M-x or
> both.  C-M-x on a defcustom or a defconst in file foo.el should not
> give a *Help* buffer that says that the var is defined in file bar.el,
> and it should not give the doc from a definition in bar.el.

AFAIK, none of those things occur in my usage.

> What's the magic to competely undo a defcustom or defconst - `makunbound'
> doesn't seem to be enough.

There's no such magic.

> Can you please give me an example of each style, for
> a context/language appropriate to that style?  Then I can try to
> suggest better one-liner descriptions.  If the behavior is different
> depending on whether comment-end is nil, then give me an example of
> each behavior.

From memory: feel free to test them in a C-mode buffer.

plain:

/*      blabla */
/*      bla */

indent

     /* blabla */
     /* bla */
     
multi-line

     /* blabla
      * bla */

extra-line      
      
     /*
      * blabla
      * bla
      */

aligned

     /* blabla */
     /* bla    */
     
box

     /**********/
     /* blabla */
     /* bla    */
     /**********/

box-multi     
     
     /**********
      * blabla *
      * bla    *
      **********/

When comment-end is the empty string,
indent=multi-line=extra-line=aligned and box=box-multi.


        Stefan

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

> > I came across some variable definition weirdness (unrelated to this
> > bug):
> 
> I must say I have no idea how you got into such a mess.  My only
> explanation would be that some part of the patch did not 
> apply properly.

No, I did not mean to suggest that this had anything to do with your patch.
(And I just "applied" the patch by hand editing.)

> AFAIK, none of those things occur in my usage.

I won't worry about it.  If I have time I'll report it.  I don't have the time
now.

> > What's the magic to competely undo a defcustom or defconst 
> > - `makunbound' doesn't seem to be enough.
> 
> There's no such magic.

Are you saying that `makunbound' should take care of it?

It's pretty easy to see at least some of the problems, I think.  Put these in
different files:

(defcustom foo 1 "The first definition"
 :type 'integer)

(defcustom foo 2 "The second definition"
 :type 'integer)

Then eval the first, then the second, then maybe the first again, using C-M-x.
Use C-h v and see if you don't see at least the wrong file name mentioned at
some points.  I do right away, using the latest Windows binary (built today by
Sean) and emacs -Q.

But all this is outside of bug #7509.  I didn't mean to suggest it had anything
to do with your patch.

> From memory: feel free to test them in a C-mode buffer.

I'll go by your memory.  Someone can correct them later if your memory isn't
perfect.

> plain:
> /*      blabla */
> /*      bla */
> 
> indent
>      /* blabla */
>      /* bla */
>      
> multi-line
>      /* blabla
>       * bla */
> 
> extra-line      
>      /*
>       * blabla
>       * bla
>       */
> 
> aligned
>      /* blabla */
>      /* bla    */
>      
> box
>      /**********/
>      /* blabla */
>      /* bla    */
>      /**********/
> 
> box-multi     
>      /**********
>       * blabla *
>       * bla    *
>       **********/
> 
> When comment-end is the empty string,
> indent=multi-line=extra-line=aligned and box=box-multi.

Thanks.  That helps a lot.  Below are some suggestions.

Also, it wouldn't hurt to include such tiny diagrams in the doc string for
`comment-styles' (esp. since it is a defconst).  In this case, a set of pictures
is worth more than a set of one-liner descriptions.

BTW, why are all of the various possibilities (align, extra, box etc.) defined
only for indented comments?

plain      - Start in column 0 (do not indent)
indent     - Full comment per line, ends not aligned
aligned    - Full comment per line, ends aligned
box        - Full comment per line, ends aligned, + top and bottom
extra-line - One comment for all lines, end on a line by itself
multi-line - One comment for all lines, end on last commented line
box-multi  - One comment for all lines, + top and bottom

And the descriptions make more sense if presented in this order, IMO.

The doc string for `comment-styles' can also point out:

1. That all except for `plain' indent the comment to the current indent level.
2. That when `comment-end' is empty...

Those general rules help understanding and recollection.

HTH.

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Stefan Monnier]

>> There's no such magic.
> Are you saying that `makunbound' should take care of it?

No.  I'm saying that there is no known trick to undo a defcustom.

> Also, it wouldn't hurt to include such tiny diagrams in the doc string
> for `comment-styles' (esp. since it is a defconst).  In this case,
> a set of pictures is worth more than a set of one-liner descriptions.

Again, that would amount to documenting the particular current value of
that variable, rather than the set of possible values and their meaning.
So that would be wrong.  Such documentation can't belong to
comment-styles's docstring; only to the doc of each style defined there.

> BTW, why are all of the various possibilities (align, extra, box etc.)
> defined only for indented comments?

The only reason `plain' exists is because that's what was used in
Emacs≤22.  I strongly discourage people from using it since it often
results in comments that aren't properly indented.

> plain      - Start in column 0 (do not indent)
> indent     - Full comment per line, ends not aligned
> aligned    - Full comment per line, ends aligned
> box        - Full comment per line, ends aligned, + top and bottom
> extra-line - One comment for all lines, end on a line by itself
> multi-line - One comment for all lines, end on last commented line
> box-multi  - One comment for all lines, + top and bottom

Thanks, that seems much better than what I have now.


        Stefan

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

> > Also, it wouldn't hurt to include such tiny diagrams in the 
> > doc string for `comment-styles' (esp. since it is a defconst).
> > In this case, a set of pictures is worth more than a set of
> > one-liner descriptions.
> 
> Again, that would amount to documenting the particular 
> current value of that variable, rather than the set of
> possible values and their meaning.

No, we would be documenting the _default_ value.  Nothing wrong with that.  It
serves as a model, a particular value, yes, but also a good example.  And the
value that defines the behavior (the possibilities) out of the box - good to
know.

Both understanding the default behavior/value and having it as a model are
useful.

> So that would be wrong.  Such documentation can't belong to
> comment-styles's docstring; only to the doc of each style 
> defined there.

Either nothing is defined there (it's just one possible value, as you point out)
or everything is defined there (it's the default value, defining all the
behavior possibilities you get if you don't change anything).  You cannot have
it both ways. ;-)

I'd say that we should describe the default value in the doc string (and not
just inside the value itself), because I think that will help users.  The doc
string just needs to make clear that this description applies to the _default
value_, which serves as _an example_.

No matter how good the one-liner descriptions we might come up with, they are no
match for the little samples you sent me.  Once you see them it's all clear, but
until then a reader is really only guessing.  IMO.

> > BTW, why are all of the various possibilities (align, 
> > extra, box etc.) defined only for indented comments?
> 
> The only reason `plain' exists is because that's what was used in
> Emacs=22.  I strongly discourage people from using it since it often
> results in comments that aren't properly indented.

I strongly prefer `plain'. ;-)  But I don't program in C.  And I don't pretend
to speak for anyone but myself.

> > plain      - Start in column 0 (do not indent)
> > indent     - Full comment per line, ends not aligned
> > aligned    - Full comment per line, ends aligned
> > box        - Full comment per line, ends aligned, + top and bottom
> > extra-line - One comment for all lines, end on a line by itself
> > multi-line - One comment for all lines, end on last commented line
> > box-multi  - One comment for all lines, + top and bottom
> 
> Thanks, that seems much better than what I have now.

You're welcome.  Thanks for making the changes.

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Stefan Monnier]

> You're welcome.  Thanks for making the changes.

I've installed the patch below, which I hope addresses your concerns
without offending my sense of modularity.


        Stefan

        
--- lisp/newcomment.el	2010-10-21 20:12:38 +0000
+++ lisp/newcomment.el	2010-12-03 23:11:10 +0000
@@ -187,21 +187,58 @@
 it is 1 so that regions are commented with two or three semi-colons.")
 
 (defconst comment-styles
-  '((plain	. (nil nil nil nil))
-    (indent	. (nil nil nil t))
-    (indent-or-triple
-                . (nil nil nil multi-char))
-    (aligned	. (nil t nil t))
-    (multi-line	. (t nil nil t))
-    (extra-line	. (t nil t t))
-    (box	. (nil t t t))
-    (box-multi	. (t t t t)))
-  "Comment region styles of the form (STYLE . (MULTI ALIGN EXTRA INDENT)).
+  '((plain      nil nil nil nil
+                "Start in column 0 (do not indent), as in Emacs-20")
+    (indent-or-triple nil nil nil multi-char
+              "Start in column 0, but only for single-char starters")
+    (indent     nil nil nil t
+                "Full comment per line, ends not aligned")
+    (aligned	nil t   nil t
+                "Full comment per line, ends aligned")
+    (box	nil t   t   t
+                "Full comment per line, ends aligned, + top and bottom")
+    (extra-line	t   nil t   t
+                "One comment for all lines, end on a line by itself")
+    (multi-line	t   nil nil t
+                "One comment for all lines, end on last commented line")
+    (box-multi	t   t   t   t
+                "One comment for all lines, + top and bottom"))
+  "Comment region style definitions.
+Each style is defined with a form (STYLE . (MULTI ALIGN EXTRA INDENT DOC)).
+DOC should succinctly describe the style.
 STYLE should be a mnemonic symbol.
 MULTI specifies that comments are allowed to span multiple lines.
+  e.g. in C it comments regions as
+     /* blabla
+      * bli */
+  rather than
+     /* blabla */
+     /* bli */
+  if `comment-end' is empty, this has no effect.
+
 ALIGN specifies that the `comment-end' markers should be aligned.
+  e.g. in C it comments regions as
+     /* blabla */
+     /* bli    */
+  rather than
+     /* blabla */
+     /* bli */
+  if `comment-end' is empty, this has no effect, unless EXTRA is also set,
+  in which case the comment gets wrapped in a box.
+  
 EXTRA specifies that an extra line should be used before and after the
   region to comment (to put the `comment-end' and `comment-start').
+  e.g. in C it comments regions as
+     /*
+      * blabla
+      * bli
+      */
+  rather than
+     /* blabla
+      * bli */
+  if the comment style is not multi line, this has no effect, unless ALIGN
+  is also set, in which case the comment gets wrapped in a box.
+
 INDENT specifies that the `comment-start' markers should not be put at the
   left margin but at the current indentation of the region to comment.
 If INDENT is `multi-char', that means indent multi-character
@@ -212,7 +249,10 @@
   "Style to be used for `comment-region'.
 See `comment-styles' for a list of available styles."
   :type (if (boundp 'comment-styles)
-	    `(choice ,@(mapcar (lambda (s) `(const ,(car s)))
+	    `(choice
+              ,@(mapcar (lambda (s)
+                          `(const :tag ,(format "%s: %s" (car s) (nth 5 s))
+                                  ,(car s)))
 			       comment-styles))
 	  'symbol)
   :version "23.1"

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Drew Adams]

Looks good.

Minor:

>  ALIGN ...
> +  if `comment-end' is empty, this has no effect, unless 
> EXTRA is also set,
> +  in which case the comment gets wrapped in a box.

You don't say what "wrapped in a box" means.  It's OK, but my guess is that
someone will just have to try this to understand.

>  EXTRA ...
> +  if the comment style is not multi line, this has no 
> effect, unless ALIGN
> +  is also set, in which case the comment gets wrapped in a box.

Likewise.  Again, it's OK, even if not real clear.

>  If INDENT is `multi-char', that means indent multi-character

Sentence should end in a period.

bug#7509: 24.0.50; doc for `comment-style' and `comment-styles'

[Stefan Monnier]

> The possible values seem to include `box' and `box-multi' (at least
> I see those in the default alist value).  Neither of those is
> described in the doc string.

Neither are plain, indent, indent-or-triple, aligned, multi-line, and
extra-line.


        Stefan