Re: custom-set-variables considered harmful

[Robert Weiner <rsw@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

On Mon, Dec 4, 2017 at 8:48 AM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Any objection to installing the patch below into `master`?
>
>         Stefan
>
>
> +(defcustom custom-mimic-plain-elisp nil

+  "If non-nil, save user settings with the new format.
> +This new format tries to mimick the code that would be written by hand
> +in plain Elisp.  But it relies on `custom-autogenerated-user-settings'
> which
> +is a new macro in Emacs-27, so settings saved with this will not work
> +reliably in Emacs<27 (although a backward compatibility trick is used
> +which should make them work to some extent)."
>


I think there must be a better way to name what you are doing here
and to explain it in the doc string.  Generally, a reader won't
know what 'new format' or 'plain Elisp' mean.  See if you can
define and clarify them.  Also, the mimic term just feels unfamiliar
vis-a-vis the rest of the Emacs code base.


> +  :type 'boolean
> +  :group 'customize)
>

​Don't we typically add '-flag​' for booleans nowadays?

​​
> +
> ​
>
> +
> ​​
> (defcustom custom-mimic-plain-elisp-use-setq nil
> +
> ​​
>   "If non-nil, use `setq' when possible in generated code.
> +
> ​​
> If nil, `custom-mimic-plain-elisp' will only use `customize-set-variable',
> ​​
> +
> ​​
> which is the more reliable option."
>

​Here the documentation is much clearer but the naming could still
be improved.  What about, `custom-use-setq-flag'?  And clarify
the contexts where this is used, does it apply to the let bindings
that RMS mentioned?


> +(defun custom--mimic-plain-elisp (symbol exp requests comment)
>

​Since this prints things, its name should mention that.
How about, 'custom--print-option-setter'?
​
Thanks for developing this and all your great work.

Bob

[-- Attachment #2: Type: text/html, Size: 5236 bytes --]