Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

[Eli Zaretskii]

> +(defcustom eglot-events-buffer-config
> +  (list :size (or (bound-and-true-p eglot-events-buffer-size) 2000000)
> +        :format 'full)
> +  "Configure the Eglot events buffer.
> +
> +Value is a plist accepting the keys `:size', which controls the
> +size in characters of the buffer (0 disables, nil means
> +infinite), and `:format', which controls the shape of each log
> +entry (`full' includes the original JSON, `lisp' uses
> +pretty-printed Lisp).
> +
> +For changes on this variable to take effect, you need to restart
> +the LSP connection.  That can be done by `eglot-reconnect'."

Since changing this defcustom needs some code to be run in order to
make the changes effective, I wonder whether this defcustom could use
:set to run that code automatically?

Thanks.

Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

[João Távora]

On Wed, Dec 27, 2023 at 4:47 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > +(defcustom eglot-events-buffer-config
> > +  (list :size (or (bound-and-true-p eglot-events-buffer-size) 2000000)
> > +        :format 'full)
> > +  "Configure the Eglot events buffer.
> > +
> > +Value is a plist accepting the keys `:size', which controls the
> > +size in characters of the buffer (0 disables, nil means
> > +infinite), and `:format', which controls the shape of each log
> > +entry (`full' includes the original JSON, `lisp' uses
> > +pretty-printed Lisp).
> > +
> > +For changes on this variable to take effect, you need to restart
> > +the LSP connection.  That can be done by `eglot-reconnect'."
>
> Since changing this defcustom needs some code to be run in order to
> make the changes effective, I wonder whether this defcustom could use
> :set to run that code automatically?

Read above in the same diff:

 @c FIXME: Shouldn't the defcustom do this by itself using the :set
-@c attribute?
+@c attribute?  Maybe not because reconnecting is a complex task.
 @xref{Troubleshooting Eglot}, for when this could be useful.

IOW Reconnecting to a server may trash useful caches, fail due
to numerous reasons, etc...  We don't want to initiate it just
because a user changed a preference.  We could ask for confirmation
though, patches welcome.

João

Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 27 Dec 2023 17:07:22 +0000
> Cc: emacs-devel@gnu.org
> 
> On Wed, Dec 27, 2023 at 4:47 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Since changing this defcustom needs some code to be run in order to
> > make the changes effective, I wonder whether this defcustom could use
> > :set to run that code automatically?
> 
> Read above in the same diff:
> 
>  @c FIXME: Shouldn't the defcustom do this by itself using the :set
> -@c attribute?
> +@c attribute?  Maybe not because reconnecting is a complex task.
>  @xref{Troubleshooting Eglot}, for when this could be useful.
> 
> IOW Reconnecting to a server may trash useful caches, fail due
> to numerous reasons, etc...  We don't want to initiate it just
> because a user changed a preference.

Shouldn't these caveats be in the doc string, where you tell users to
invoke eglot-reconnect?

Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

[João Távora]

On Wed, Dec 27, 2023 at 5:29 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > IOW Reconnecting to a server may trash useful caches, fail due
> > to numerous reasons, etc...  We don't want to initiate it just
> > because a user changed a preference.
>
> Shouldn't these caveats be in the doc string, where you tell users to
> invoke eglot-reconnect?

No.  It's too server-specific, every server can behave differently.  It's
assumed that if a user wants to reconnect and interactively chooses to do
that, it's because they want to and understand what it does.  The model
is the same as in SLIME and SLY, which also work with a multitude of CL
implementations, and it has never proven problematic to my knowledge
(at least in a over a decade of SLY maintenance).

João

Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 27 Dec 2023 17:38:13 +0000
> Cc: emacs-devel@gnu.org
> 
> On Wed, Dec 27, 2023 at 5:29 PM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > > IOW Reconnecting to a server may trash useful caches, fail due
> > > to numerous reasons, etc...  We don't want to initiate it just
> > > because a user changed a preference.
> >
> > Shouldn't these caveats be in the doc string, where you tell users to
> > invoke eglot-reconnect?
> 
> No.  It's too server-specific, every server can behave differently.  It's
> assumed that if a user wants to reconnect and interactively chooses to do
> that, it's because they want to and understand what it does.  The model
> is the same as in SLIME and SLY, which also work with a multitude of CL
> implementations, and it has never proven problematic to my knowledge
> (at least in a over a decade of SLY maintenance).

Then we are inconsistent: either running eglot-reconnect is mostly
okay, in which case we should do it automatically when the option's
value changes, or it has issues, in which case we should at least hint
on those issues in the doc string, perhaps saying that with some
servers it's okay and with others less so.  The way the doc string is
worded now, users will invoke eglot-reconnect without being aware of
the possible issues which are the reason that you are reluctant to
invoke eglot-reconnect automatically.

Re: master d9b5f618baa 2/4: Eglot: introduce eglot-events-buffer-config

[João Távora]

On Wed, Dec 27, 2023 at 6:52 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Wed, 27 Dec 2023 17:38:13 +0000
> > Cc: emacs-devel@gnu.org
> >
> > On Wed, Dec 27, 2023 at 5:29 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > > IOW Reconnecting to a server may trash useful caches, fail due
> > > > to numerous reasons, etc...  We don't want to initiate it just
> > > > because a user changed a preference.
> > >
> > > Shouldn't these caveats be in the doc string, where you tell users to
> > > invoke eglot-reconnect?
> >
> > No.  It's too server-specific, every server can behave differently.  It's
> > assumed that if a user wants to reconnect and interactively chooses to do
> > that, it's because they want to and understand what it does.  The model
> > is the same as in SLIME and SLY, which also work with a multitude of CL
> > implementations, and it has never proven problematic to my knowledge
> > (at least in a over a decade of SLY maintenance).
>
> Then we are inconsistent: either running eglot-reconnect is mostly
> okay, in which case we should do it automatically when the option's
> value changes, or it has issues, in which case we should at least hint
> on those issues in the doc string, perhaps saying that with some
> servers it's okay and with others less so.  The way the doc string is
> worded now, users will invoke eglot-reconnect without being aware of
> the possible issues which are the reason that you are reluctant to
> invoke eglot-reconnect automatically.

No.  What a connection entails is explained in more than sufficient
detail already. Stuff like this, which requires network resources,
reads config files, runs user hooks etc is just not good to invoke
non-interactively is all.  Not to mention a user may have multiple
connections ongoing for multiple connections, some quick to connect,
some slow, some sync, some async (see doc of eglot-connect-timeout).

I just don't think the problem exists.  Moreover the number one
reason for garden-variety users to want to customize that otherwise
obscure variable is now likely gone, since the default value leads
to much faster performance.

João