Subject: Re: bug#63861: [PATCH] pp.el: New "pretty printing" code

From: Stefan Monnier <[email-masked]>
Date: Sat, 03 Jun 2023 14:18:36 -0400

>> I've often been annoyed by the way `ielm` "pretty prints" data,
>> so I wrote my own pretty printer, which has evolved over the years.
>> I believe it has finally reached a state which may be acceptable
>> to more than just myself.
>> 
>> The new code is in a new function `pp-region`.
>> The old code redirects to the new code if `pp-buffer-use-pp-region` is
>> non-nil, tho I'm not sure we want to bother users with such
>> a config var.  Hopefully, the new code should be good enough that users
>> don't need to choose.  Maybe I should make it a `defvar` and have it
>> default to t, so new users will complain if it's not good enough?
>
> Thanks.  I almost never use IELM, so I have no significant comments to
> this, only minor ones.

FWIW, the change affects other functionality that uses `pp`, such as
`C-h v`.  While working on (previous versions of) this code, I've had
performance problems show up during the generation of `emoji-labels.el`.

>> +(defun pp-region (beg end)
>> +  "Insert newlines in BEG..END to try and fit within `fill-column'.
>> +Presumes the current buffer contains Lisp code and has indentation properly
>> +configured for that.
>> +Designed under the assumption that the region occupies a single line,
>> +tho it should also work if that's not the case."
>
> The first line should say what this command does.

How 'bout:

    Insert line-breaks in Lisp code so it fits within `fill-column`.

?

> Also, I think this warrants a NEWS entry and should be documented in
> the ELisp manual.

Definitely for NEWS, yes.  For the ELisp manual, currently we don't
document `pp-buffer`, the closest I see is `indent-pp-sexp` (in
`programs.texi`).
I'm not sure what to put in there. nor where to put it.

>> +(defcustom pp-buffer-use-pp-region nil
>> +  "If non-nil, `pp-buffer' uses the new `pp-region' code."
>> +  :type 'boolean)
> Please add :version.

Hmm... so you think it should stay as a `defcustom` and we should thus
plan to keep both kinds of pretty-printing in the long term?
I mostly intended it to be a temporary knob for people to be able to try
the new code and easily compare with the old (or revert to the old when
bumping into a problem with the new).

If so, we should probably think of better names to distinguish the two
pp styles than `pp-buffer` vs `pp-region`.  Maybe `pp-fill` for the new
code since arguably the main difference is that the new code pays
attention to `fill-column`?  I don't have a good idea for a name for the
old code, OTOH (and I think it would make sense to keep `pp-buffer` as
a dispatch between the two options, so it would be good to have
a separate name for the old style).

Another difference might be that the new style is maybe aimed more at
pp'ing code than data, whereas the old style might be a bit more
"agnostic" to the definition.  Yet another difference is that the old
code tends to use more lines (because it doesn't try to fill the line
upto `fill-column`) and occasionally outputs very long lines because it
only breaks lines near parentheses.

Maybe that info can inspire someone to come up with a good name for this
"old style"?

> Also, "the new code" should be rephrased to not use "new" (which
> doesn't stand the time test).

:-)

> And the new defcustom should probably be mentioned in the manual,
> perhaps where we mention IELM.

If it stays as a `defcustom`, agreed.

        Stefan