bug#10868: 24.0.93; `comment-style' needs to be explained

bug#10868: 24.0.93; `comment-style' needs to be explained

[Drew Adams]

This is a followup to bug #3370, which was closed without attention to
the bug thread.  The problem is that variable `comment-style' is not
explained.  Please do so.
 
See this message from the bug #3370 thread, as well as other parts of
the thread, for a discussion of the problem:
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=3370#88


In GNU Emacs 24.0.93.1 (i386-mingw-nt5.1.2600)
 of 2012-02-15 on MARVIN
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.6) --no-opt --enable-checking --cflags
 -ID:/devel/emacs/libs/libXpm-3.5.8/include
 -ID:/devel/emacs/libs/libXpm-3.5.8/src
 -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include
 -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include
 -ID:/devel/emacs/libs/giflib-4.1.4-1/include
 -ID:/devel/emacs/libs/jpeg-6b-4/include
 -ID:/devel/emacs/libs/tiff-3.8.2-1/include
 -ID:/devel/emacs/libs/gnutls-3.0.9/include'
 

bug#10868: 24.0.93; `comment-style' needs to be explained

[Glenn Morris]

> The problem is that variable `comment-style' is not explained.

comment-style is a variable defined in `newcomment.el'.
Its value is indent

Documentation:
Style to be used for `comment-region'.
See `comment-styles' for a list of available styles.


comment-styles is a variable defined in `newcomment.el'.
[...]
((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"))

bug#10868: 24.0.93; `comment-style' needs to be explained

[Drew Adams]

> > The problem is that variable `comment-style' is not explained.
> 
> comment-style is a variable defined in `newcomment.el'.
> Its value is indent
> 
> Documentation:
> Style to be used for `comment-region'.
> See `comment-styles' for a list of available styles.
> 
> comment-styles is a variable defined in `newcomment.el'.
> [...]
> ((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"))

You are just parroting the existing, insufficient doc.

Read the bug #3370 thread, if you want to understand the problem.  There is no
_explanation_ of `comment-style'.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Glenn Morris]

> Read the bug #3370 thread, if you want to understand the problem.

Unfortunately, I did. It is long-winded and goes nowhere.
I encourage everyone else not to bother.

> There is no _explanation_ of `comment-style'.

Right, well, good luck with your future bug reporting.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Drew Adams]

 
> > Read the bug #3370 thread, if you want to understand the problem.
> 
> Unfortunately, I did. It is long-winded and goes nowhere.
> I encourage everyone else not to bother.

Is it really so hard to understand?

 "the doc string for `comment-style' (which is pretty much
  useless) refers to `comment-styles', which is not
  recognized (and therefore has no link)."

emacs -Q

C-h v comment-style

 Style to be used for `comment-region'.
 See `comment-styles' for a list of available styles.

C-h v comment-styles

Bzzzzt -not recognized - no such variable, no such "list of available styles"
... until you load newcomment.el.

There is a user option (`comment-style'), which you can customize.  But its doc
tells you nothing useful.  It just refers you to an internal variable
(`comment-styles'), which has no doc until you load its library.

The doc for `comment-styles' _is_ helpful ... once it becomes available.

Here is a case where we actually autoload a user option, `comment-style', so its
(useless) doc is available prior to loading the library.

But that doc string just refers you to a variable (constant, actually) whose doc
(the advertised "list of available styles") is not available until you load the
library.

Bravo.  Thanks for helping users out.

> > There is no _explanation_ of `comment-style'.
> 
> Right, well, good luck with your future bug reporting.

So constructive.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Lawrence Mitchell]

Drew Adams wrote:
>>> Read the bug #3370 thread, if you want to understand the problem.

>> Unfortunately, I did. It is long-winded and goes nowhere.
>> I encourage everyone else not to bother.

> Is it really so hard to understand?

>  "the doc string for `comment-style' (which is pretty much
>   useless) refers to `comment-styles', which is not
>   recognized (and therefore has no link)."

> emacs -Q

> C-h v comment-style

>  Style to be used for `comment-region'.
>  See `comment-styles' for a list of available styles.

> C-h v comment-styles

> Bzzzzt -not recognized - no such variable, no such "list of available styles"
> ... until you load newcomment.el.

[...]

OK, here's the problem, and here's a patch.

2012-02-23  Lawrence Mitchell <wence@gmx.li>

* lisp/newcomment.el: Autoload comment-styles (Bug#3370).

The docstring of comment-style refers the user to comment-styles for
details on what the values mean.  However, this is not available until
after newcomment.el is loaded.  So autoload comment-styles to avoid
this problem.
---
 lisp/newcomment.el |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index 16282af..cd887cb 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -185,6 +185,7 @@ The `plain' comment style doubles this value.
 This should generally stay 0, except for a few modes like Lisp where
 it is 1 so that regions are commented with two or three semi-colons.")
 
+;;;###autoload
 (defconst comment-styles
   '((plain      nil nil nil nil
                 "Start in column 0 (do not indent), as in Emacs-20")
-- 
1.7.9.rc0.23.g7e521

bug#10868: 24.0.93; `comment-style' needs to be explained

[Stefan Monnier]

> OK, here's the problem, and here's a patch.
> 2012-02-23  Lawrence Mitchell <wence@gmx.li>
> * lisp/newcomment.el: Autoload comment-styles (Bug#3370).

Another fix is to remove the ;;;###autoload from comment-style.
If that doesn't introduce other problems, it's a better solution.

Of course, yet another solution is to preload newcomment.el (an option
that was rejected back when newcomment.el was introduced into Emacs-21).


        Stefan

bug#10868: 24.0.93; `comment-style' needs to be explained

[Drew Adams]

> > OK, here's the problem, and here's a patch.
> > 2012-02-23  Lawrence Mitchell <wence@gmx.li>
> > * lisp/newcomment.el: Autoload comment-styles (Bug#3370).
> 
> Another fix is to remove the ;;;###autoload from comment-style.
> If that doesn't introduce other problems, it's a better solution.
> 
> Of course, yet another solution is to preload newcomment.el (an option
> that was rejected back when newcomment.el was introduced into 
> Emacs-21).

FWIW, I believe I'm OK with any of the 3 proposed solutions.
Haven't thought about it much, but they all seem OK.

Typically, Stefan, you do not like `;;;##autoload' cookies for user options (at
least this has been argued before).  Is there a reason here why you might prefer
one of the other solutions?  What "other problems" are you thinking of?

bug#10868: 24.0.93; `comment-style' needs to be explained

[Stefan Monnier]

> Typically, Stefan, you do not like `;;;##autoload' cookies for user
> options (at least this has been argued before).  Is there a reason
> here why you might prefer one of the other solutions?

I like preloading newcomment.el.  It's a package that is very likely to
be used in most Emacs sessions, and it has a whole bunch of autoload
cookies in it (and prior to introducing newcomment.el, the corresponding
functionality was preloaded since it was in simple.el).

> What "other problems" are you thinking of?

The ones I can't think of ;-)


        Stefan

bug#10868: 24.0.93; `comment-style' needs to be explained

[Drew Adams]

> I like preloading newcomment.el.  It's a package that is very 
> likely to be used in most Emacs sessions, and it has a whole
> bunch of autoload cookies in it (and prior to introducing
> newcomment.el, the corresponding functionality was preloaded
> since it was in simple.el).

Sounds good to me.  And it's good to know those (3 strong) reasons, for future
contexts.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Chong Yidong]

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

>> OK, here's the problem, and here's a patch.
>> 2012-02-23  Lawrence Mitchell <wence@gmx.li>
>> * lisp/newcomment.el: Autoload comment-styles (Bug#3370).
>
> Another fix is to remove the ;;;###autoload from comment-style.
> If that doesn't introduce other problems, it's a better solution.

The docstring of comment-region refers to comment-style, so removing the
autoload of comment-style brings us back to square one.

I added an autoload for comment-styles.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Glenn Morris]

Lawrence Mitchell wrote:

> The docstring of comment-style refers the user to comment-styles for
> details on what the values mean.  However, this is not available until
> after newcomment.el is loaded.  So autoload comment-styles to avoid
> this problem.

Then this is an instance of a general problem. See eg
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1768
for the same thing.

IMO there should be a general solution rather than papering over every
instance that occurs with more pre/autoloading.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Glenn Morris]

Glenn Morris wrote:

> Then this is an instance of a general problem. See eg

PS Someone might want to rename this report to reflect the real issue.

bug#10868: 24.0.93; `comment-style' needs to be explained

[Lawrence Mitchell]

Glenn Morris wrote:
> Lawrence Mitchell wrote:

>> The docstring of comment-style refers the user to comment-styles for
>> details on what the values mean.  However, this is not available until
>> after newcomment.el is loaded.  So autoload comment-styles to avoid
>> this problem.

> Then this is an instance of a general problem. See eg
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1768
> for the same thing.

> IMO there should be a general solution rather than papering over every
> instance that occurs with more pre/autoloading.

Sure, but you can't do this, right?  You don't know where the
variable is defined until /after/ the file it's defined in is
loaded.  Sure, you can guess that links in a docstring for a
variable that is autoloaded from package foo may also lead to
other variables in package foo, but that's not foolproof.

I guess the help system could load the file of autoloaded
variables when showing their docstring, but people are bound to
complain about that too.

Lawrence

bug#10868: 24.0.93; `comment-style' needs to be explained

[Drew Adams]

> > The docstring of comment-style refers the user to comment-styles for
> > details on what the values mean.  However, this is not available
> > until after newcomment.el is loaded.  So autoload comment-styles
> > to avoid this problem.
> 
> Then this is an instance of a general problem. See eg
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1768
> for the same thing.
> 
> IMO there should be a general solution rather than papering over every
> instance that occurs with more pre/autoloading.

Maybe, maybe not.

No one has suggested "papering over *every instance* that occurs with more
pre/autoloading".  Quite the contrary.  Stefan gave some specific guidelines
(reasons) why preloading makes sense in this _particular_ case.

But if what you say is true and is agreed upon, we can discuss the general
problem & solution separately - in emacs-devel, for example.

Wrt _this_ bug report: let us not let the perfect become the enemy of the good.
Let's not wait for some expected or hoped-for general solution before fixing
this particular bug.

It's hard enough to get approval that a particular option's doc string be
preloaded.  Let's not throw abstract obstacles in the path, once that need has
been recognized in a particular case.

> PS Someone might want to rename this report to reflect the real issue.

Please, no.  Please do not try to detour this bug fix.

Instead, file a separate bug report for the general case that you suppose.  Or
better yet, bring up the question for discussion at emacs-devel.

Finding a variable before it's loaded

[Stefan Monnier]

>> The docstring of comment-style refers the user to comment-styles for
>> details on what the values mean.  However, this is not available until
>> after newcomment.el is loaded.  So autoload comment-styles to avoid
>> this problem.
> Then this is an instance of a general problem. See eg
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1768
> for the same thing.
> IMO there should be a general solution rather than papering over every
> instance that occurs with more pre/autoloading.

Yes, there is a general problem underneath, but it's difficult to
solve satisfactorily.  IIRC someone did submit a potential solution at
some point, but it required building a fairly large file listing where
each var is defined.

I have toyed with a slightly different approach which tries to reduce
the size of this file by keeping not the list of all vars, but only
a list of prefixes used by each file.
This way, if I want to find `comment-styles', I can simply load all the files
that define variables with the "comment-" prefix.

The table of prefixes is not very large, so I can simply have it
preloaded and put it in loaddefs.el (and it's automatically maintained
by autoload.el, which means it can also work for non-bundled packages if
their autoloads are generated by autoload.el).

E.g. for newcomment.el my code adds:

(register-definition-prefixes "newcomment"
  '("comment-" "uncomment-region-function" "uncomment-region-default"
    "block-comment-start" "block-comment-end"))

and for diff-mode.el it doesn't add anything at all, because I use
a default rule "for <foo>-<bar>, look for a file <foo>.el or
<foo>-mode.el" and that covers all the definitions in diff-mode.el.

It's also fun to look at all the register-definition-prefixes in
loaddefs.el as a way to flag "unclean namespace issues".


        Stefan

Re: Finding a variable before it's loaded

[Lennart Borgman]

On Thu, Feb 23, 2012 at 22:48, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>> The docstring of comment-style refers the user to comment-styles for
>>> details on what the values mean.  However, this is not available until
>>> after newcomment.el is loaded.  So autoload comment-styles to avoid
>>> this problem.
>> Then this is an instance of a general problem. See eg
>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1768
>> for the same thing.
>> IMO there should be a general solution rather than papering over every
>> instance that occurs with more pre/autoloading.
>
> Yes, there is a general problem underneath, but it's difficult to
> solve satisfactorily.  IIRC someone did submit a potential solution at
> some point, but it required building a fairly large file listing where
> each var is defined.

I did that. Though I did not have time to finish it in the end.
However the timing was not a big problem. The table size was perhaps a
problem.

> I have toyed with a slightly different approach which tries to reduce
> the size of this file by keeping not the list of all vars, but only
> a list of prefixes used by each file.

Sounds like a better solution.

> This way, if I want to find `comment-styles', I can simply load all the files
> that define variables with the "comment-" prefix.

Could not that be problematic? Maybe the build process could warn if a
prefix is found in the wrong file?