bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Drew Adams]

The doc string should suggest that users use Customize. It should not
use a complex `setq' example as its only illustration:
 
(setq dired-guess-shell-alist-user
      (list (list "\\.foo\\'" "FOO-COMMAND");; fixed rule
             ;; possibly more rules ...
             (list "\\.bar\'";; rule with condition test
                   '(if condition
                         "BAR-COMMAND-1"
                       "BAR-COMMAND-2"))))
 
This example is in any case incorrect - "\\.bar\'" should be "\\.bar\\'".
 
If it's felt that an example of a _value_ for this option is needed,
then it's OK to show that directly:
 
(("\\.foo\\'" "foo-command") ; unconditional rule
 ("\\.bar\\'"                ; conditional rule
  (if (some-sexp) "bar-command-1" "bar-command-2")))
 
But there is absolutely no reason to show setting the value using
`setq', especially since the expression evaluated by `setq' is 100%
constant. Nothing is gained by showing anything other than the result
of that evaluation, that is, a possible value for the option.
 
It might also be better to write `(some-sexp)' or similar, instead of
`condition', to emphasise that even that part is code to be evaluated.
 
The use of uppercase for the command names is also problematic - those
are constants (strings). This is an example, not a template.
 
In sum: (1) the doc string is confusing in several respects; (2) it is
incorrect; (3) it is unnecessarily complex; and (4) it favors Lisp
instead of Customize for a user-option example.
 

 

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Lars Magne Ingebrigtsen]

"Drew Adams" <drew.adams@oracle.com> writes:

> The doc string should suggest that users use Customize. It should not
> use a complex `setq' example as its only illustration:
>
> (setq dired-guess-shell-alist-user
>       (list (list "\\.foo\\'" "FOO-COMMAND");; fixed rule
>              ;; possibly more rules ...
>              (list "\\.bar\'";; rule with condition test
>                    '(if condition
>                          "BAR-COMMAND-1"
>                        "BAR-COMMAND-2"))))

I think complicated variables are best served with non-Customize
examples.

I've rewritten it to use a quote instead of all the `list' operations,
which should make it clearer.

> This example is in any case incorrect - "\\.bar\'" should be "\\.bar\\'".

This was apparently fixed already.

> If it's felt that an example of a _value_ for this option is needed,
> then it's OK to show that directly:
>
> (("\\.foo\\'" "foo-command") ; unconditional rule
>  ("\\.bar\\'"                ; conditional rule
>   (if (some-sexp) "bar-command-1" "bar-command-2")))
>
> But there is absolutely no reason to show setting the value using
> `setq', especially since the expression evaluated by `setq' is 100%
> constant.

I disagree.  A complete `setq' is convenient to cut and paste.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Drew Adams]

Lars, you marked this bug `fixed', but it seems that it was hardly addressed
(partly addressed).  Please take another look at it.

> > The doc string should suggest that users use Customize. It 
> > should not use a complex `setq' example as its only illustration:

Note: _only_.

> > (setq dired-guess-shell-alist-user
> >       (list (list "\\.foo\\'" "FOO-COMMAND");; fixed rule
> >              ;; possibly more rules ...
> >              (list "\\.bar\'";; rule with condition test
> >                    '(if condition
> >                          "BAR-COMMAND-1"
> >                        "BAR-COMMAND-2"))))
> 
> I think complicated variables are best served with non-Customize
> examples.

It's OK to have a code example, if that helps make things clear.

And yes, it's not obvious how to _show_ Customize in the doc.  But that's not
what I suggested.  The "doc string should suggest that users use Customize."
That's the point.

It is one thing to say that _showing_ Customize in the doc is not easy and
probably not worth it.  It is another thing that the doc _only_ suggest to users
to use `setq'.

And even for a code example it would be better to use `customize-save-variable'
instead of `setq'.  We should generally encourage this practice, since for many
user options it makes a difference:

 If VARIABLE has a `custom-set' property, that is used for setting
 VARIABLE, otherwise `set-default' is used.

 If VARIABLE has a `variable-interactive' property, that is used as if
 it were the arg to `interactive' (which see) to interactively read the value.

 If VARIABLE has a `custom-type' property, it must be a widget and the
 `:prompt-value' property of that widget will be used for reading the value.

> I've rewritten it to use a quote instead of all the `list' operations,
> which should make it clearer.

That's fine, but it does not respond to the bug report at all - it's something
else.

Please suggest to users that they use Customize.  It's about the users, not our
ease in writing the doc and its examples.  I agree that it is not easy or
worthwhile to show an illustration of Customize here.  But we should
nevertheless suggest using Customize first, and show a code example only second,
if important, as a way to code things by hand.

> > If it's felt that an example of a _value_ for this option is needed,
> > then it's OK to show that directly:
> >
> > (("\\.foo\\'" "foo-command") ; unconditional rule
> >  ("\\.bar\\'"                ; conditional rule
> >   (if (some-sexp) "bar-command-1" "bar-command-2")))
> >
> > But there is absolutely no reason to show setting the value using
> > `setq', especially since the expression evaluated by `setq' is 100%
> > constant.
> 
> I disagree.  A complete `setq' is convenient to cut and paste.

That's not something we necessarily want to encourage.  Customize is much to be
preferred when it is appropriate.  It is safer, does type-checking, etc.

It's not because some of us find the Customize UI to be ugly that we should not
encourage its use.

What's important for the illustration is what a value looks like.  I recommend
that we (a) remove `setq', (2) not bother with `customize-save-variable', but
(3) show the example _value_, as I wrote initially. 

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Lars Magne Ingebrigtsen]

"Drew Adams" <drew.adams@oracle.com> writes:

> And yes, it's not obvious how to _show_ Customize in the doc.  But that's not
> what I suggested.  The "doc string should suggest that users use Customize."
> That's the point.

I disagreed with that part, but fixed the part I agreed with.  So I
think "fixed" covers it.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Drew Adams]

> > And yes, it's not obvious how to _show_ Customize in the 
> > doc.  But that's not what I suggested.  The "doc string should
> > suggest that users use Customize."  That's the point.
> 
> I disagreed with that part, but fixed the part I agreed with.  So I
> think "fixed" covers it.

No, "fixed" does not cover it.  The main point of the bug is to suggest
Customize to users.

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Lars Magne Ingebrigtsen]

"Drew Adams" <drew.adams@oracle.com> writes:

> No, "fixed" does not cover it.  The main point of the bug is to suggest
> Customize to users.

For this variable, I disagree, and I'm closing this report.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

bug#2030: 23.0.60; doc string of dired-guess-shell-alist-user

[Drew Adams]

> I disagree, and I'm closing this report.

Then please change the status from `fixed' to `wontfix', Lars.

You did not fix anything that was reported.

And there were several points raised - all ignored.  ("\\.bar\'" to "\\.bar\\'"
had apparently already been corrected.)  You even left the inappropriate
uppercase for the fictitious command names.

All you did AFAICT was change the calls to `list' to a quoted list constant - no
relation to the bug report.  Worse - part of the bug report was precisely that
there is no sense in showing an example with `setq', especially if the value is
a _constant_.

There is no reason to show _setting_ the value - in any way.  It is fine to show
an example value though (provided it is cleaned up, as suggested), if it helps
to understand the structure etc.  But no reason to show how to set it.

If for some reason (and there is none here) we did need to mention how to set
this option's value, then general Emacs policy calls for privileging Customize
in the doc.

You not only insisted that the doc string show how to set the value (useless),
but that it continue to do so using `setq' - not even `customize-set-variable'.
The reason you gave was that users will want to copy+paste this `setq'
expression.  Hard to believe.

It's your prerogative not to fix the bug, but in that case please change the
status to `wontfix'.  Thank you.