Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[João Távora]

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

Hello Stephen,

This is a relatively minor nit, but please, in future commits to the file
lisp/progmodes/eglot.el (and maybe other files), try to ensure that
whitespace
which is unrelated to the thing being fixed or added does not creep in.  It
makes browsing the history of the file (which I've taken some care to
preserve)
much easier.

Personally, I'm quite OK with reviewing whitespace-only cosmetic patches to
that
file, as long as they are in separate commits.

If in doubt, please run the final patch by me.

João


On Tue, Oct 25, 2022 at 10:18 AM Stephen Leake <
stephen_leake@stephe-leake.org> wrote:

> branch: master
> commit 31945b6c3fcbdb6f242f0063811d2fb91e4520cd
> Author: Stephen Leake <stephen_leake@stephe-leake.org>
> Commit: Stephen Leake <stephen_leake@stephe-leake.org>
>
>     * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list
> ---
>  lisp/progmodes/eglot.el | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 71001ba680..432631691c 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -229,7 +229,7 @@ language-server/bin/php-language-server.php"))
>                                  (html-mode . ,(eglot-alternatives
> '(("vscode-html-language-server" "--stdio") ("html-languageserver"
> "--stdio"))))
>                                  (json-mode . ,(eglot-alternatives
> '(("vscode-json-language-server" "--stdio") ("json-languageserver"
> "--stdio"))))
>                                  (dockerfile-mode . ("docker-langserver"
> "--stdio"))
> -                                ((clojure-mode clojurescript-mode
> clojurec-mode)
> +                                ((clojure-mode clojurescript-mode
> clojurec-mode)
>                                   . ("clojure-lsp"))
>                                  (csharp-mode . ("omnisharp" "-lsp"))
>                                  (purescript-mode .
> ("purescript-language-server" "--stdio"))
> @@ -1078,6 +1078,7 @@ MANAGED-MAJOR-MODE, which matters to a minority of
> servers.
>
>  INTERACTIVE is t if called interactively."
>    (interactive (append (eglot--guess-contact t) '(t)))
> +  (setq managed-major-mode (eglot--ensure-list managed-mode))
>    (let* ((current-server (eglot-current-server))
>           (live-p (and current-server (jsonrpc-running-p current-server))))
>      (if (and live-p
> @@ -2898,7 +2899,7 @@ for which LSP on-type-formatting should be
> requested."
>  (defun eglot--hover-info (contents &optional _range)
>    (mapconcat #'eglot--format-markup
>               (if (vectorp contents) contents (list contents)) "\n"))
> -
> +
>  (defun eglot--sig-info (sigs active-sig sig-help-active-param)
>    (cl-loop
>     for (sig . moresigs) on (append sigs nil) for i from 0
>
>

-- 
João Távora

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

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[João Távora]

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

Also this patch breaks M-x eglot completely (!), hehe but that was
another type of oversight that I myself commit more than occasionally.

I've just fixed it.

João

On Tue, Oct 25, 2022 at 10:29 AM João Távora <joaotavora@gmail.com> wrote:

> Hello Stephen,
>
> This is a relatively minor nit, but please, in future commits to the file
> lisp/progmodes/eglot.el (and maybe other files), try to ensure that
> whitespace
> which is unrelated to the thing being fixed or added does not creep in.
> It
> makes browsing the history of the file (which I've taken some care to
> preserve)
> much easier.
>
> Personally, I'm quite OK with reviewing whitespace-only cosmetic patches
> to that
> file, as long as they are in separate commits.
>
> If in doubt, please run the final patch by me.
>
> João
>
>
> On Tue, Oct 25, 2022 at 10:18 AM Stephen Leake <
> stephen_leake@stephe-leake.org> wrote:
>
>> branch: master
>> commit 31945b6c3fcbdb6f242f0063811d2fb91e4520cd
>> Author: Stephen Leake <stephen_leake@stephe-leake.org>
>> Commit: Stephen Leake <stephen_leake@stephe-leake.org>
>>
>>     * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list
>> ---
>>  lisp/progmodes/eglot.el | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
>> index 71001ba680..432631691c 100644
>> --- a/lisp/progmodes/eglot.el
>> +++ b/lisp/progmodes/eglot.el
>> @@ -229,7 +229,7 @@ language-server/bin/php-language-server.php"))
>>                                  (html-mode . ,(eglot-alternatives
>> '(("vscode-html-language-server" "--stdio") ("html-languageserver"
>> "--stdio"))))
>>                                  (json-mode . ,(eglot-alternatives
>> '(("vscode-json-language-server" "--stdio") ("json-languageserver"
>> "--stdio"))))
>>                                  (dockerfile-mode . ("docker-langserver"
>> "--stdio"))
>> -                                ((clojure-mode clojurescript-mode
>> clojurec-mode)
>> +                                ((clojure-mode clojurescript-mode
>> clojurec-mode)
>>                                   . ("clojure-lsp"))
>>                                  (csharp-mode . ("omnisharp" "-lsp"))
>>                                  (purescript-mode .
>> ("purescript-language-server" "--stdio"))
>> @@ -1078,6 +1078,7 @@ MANAGED-MAJOR-MODE, which matters to a minority of
>> servers.
>>
>>  INTERACTIVE is t if called interactively."
>>    (interactive (append (eglot--guess-contact t) '(t)))
>> +  (setq managed-major-mode (eglot--ensure-list managed-mode))
>>    (let* ((current-server (eglot-current-server))
>>           (live-p (and current-server (jsonrpc-running-p
>> current-server))))
>>      (if (and live-p
>> @@ -2898,7 +2899,7 @@ for which LSP on-type-formatting should be
>> requested."
>>  (defun eglot--hover-info (contents &optional _range)
>>    (mapconcat #'eglot--format-markup
>>               (if (vectorp contents) contents (list contents)) "\n"))
>> -
>> +
>>  (defun eglot--sig-info (sigs active-sig sig-help-active-param)
>>    (cl-loop
>>     for (sig . moresigs) on (append sigs nil) for i from 0
>>
>>
>
> --
> João Távora
>


-- 
João Távora

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

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Personally, I'm quite OK with reviewing whitespace-only cosmetic patches to
  > that
  > file, as long as they are in separate commits.

I think that is a good approach for any program.
Whitespaces fixes that clean up the code are useful,
but it's better to keep them separate from other changes.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: emacs-devel@gnu.org, stephen_leake@stephe-leake.org
> Date: Thu, 27 Oct 2022 16:13:03 -0400
> 
>   > Personally, I'm quite OK with reviewing whitespace-only cosmetic patches to
>   > that
>   > file, as long as they are in separate commits.
> 
> I think that is a good approach for any program.
> Whitespaces fixes that clean up the code are useful,
> but it's better to keep them separate from other changes.

Our project-wide preference is the other way around: we ask
contributors not to make any whitespace changes except where they
actually change code, or nearby.

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[João Távora]

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

On Fri, Oct 28, 2022 at 6:42 AM Eli Zaretskii <eliz@gnu.org> wrote:

> >   > Personally, I'm quite OK with reviewing whitespace-only cosmetic
> patches to
> >   > that
> >   > file, as long as they are in separate commits.
> > I think that is a good approach for any program.
> > Whitespaces fixes that clean up the code are useful,
> > but it's better to keep them separate from other changes.
> Our project-wide preference is the other way around: we ask
> contributors not to make any whitespace changes except where they
> actually change code, or nearby.
>

I wouldn't call it "the other way around". The two preferences
are close in spirit, in that they both advise against whitespace
changes unrelated or far from the places where the program's
syntax tree was changed.  In both cases, intelligible history
is more easily preserved.

It's just that my (and seemingly Richard's) preference is a little more
lax. If something is definitely off cosmetically (say, very long lines
or misleadingly hard to read indentation), then a separate commit
that targets those cosmetics shortcomings is acceptable.

João

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

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 28 Oct 2022 09:32:08 +0100
> Cc: rms@gnu.org, emacs-devel@gnu.org, stephen_leake@stephe-leake.org
> 
> It's just that my (and seemingly Richard's) preference is a little more
> lax. If something is definitely off cosmetically (say, very long lines 
> or misleadingly hard to read indentation), then a separate commit
> that targets those cosmetics shortcomings is acceptable.

I'm okay with your preferences in the package for whose maintenance
you are responsible.  But people who read this list should be aware of
our project-wide preferences, lest they somehow interpret what you and
Richard say as what they need to follow in general.  This list is read
by many people, not just the three of us.

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Our project-wide preference is the other way around: we ask
> contributors not to make any whitespace changes except where they
> actually change code, or nearby.

AFAIK, the reasons not to do cosmetic whitespace changes is that they
make history harder to read, and merging harder.

However, any reasonably modern VCS will have an option to ignore
whitespace changes (Git does).  And a lot of code in Emacs change very
infrequently.  At the same time, whitespace changes can in some cases
make the structure of code clearer, and thereby easier to understand.

I therefore think we could relax our project-wide policy along the lines
of what Richard and João suggest.

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 28 Oct 2022 12:45:58 -0700
> Cc: joaotavora@gmail.com, emacs-devel@gnu.org, stephen_leake@stephe-leake.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Our project-wide preference is the other way around: we ask
> > contributors not to make any whitespace changes except where they
> > actually change code, or nearby.
> 
> AFAIK, the reasons not to do cosmetic whitespace changes is that they
> make history harder to read, and merging harder.

They also make patches harder to review, sometimes much harder.

> However, any reasonably modern VCS will have an option to ignore
> whitespace changes (Git does).  And a lot of code in Emacs change very
> infrequently.  At the same time, whitespace changes can in some cases
> make the structure of code clearer, and thereby easier to understand.
> 
> I therefore think we could relax our project-wide policy along the lines
> of what Richard and João suggest.

Not as long as the "diff" operation of the VCS treats whitespace as
significant by default.  Presumably, they do this for a reason, and
therefore patches we get to review do not ignore whitespace.

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> They also make patches harder to review, sometimes much harder.

Fully agreed.  That's why I'd prefer it if people would make whitespace
changes separately from functional changes.  Our current rule
unfortunately seems to encourage putting it all in the same patch.

Personally, I try to edit out most whitespace changes from my patches.
But occasionally, the temptation of fixing some egregious indentation is
too big, also for me.

> Not as long as the "diff" operation of the VCS treats whitespace as
> significant by default.  Presumably, they do this for a reason, and
> therefore patches we get to review do not ignore whitespace.

It's hard to hope for a change in that default in Git itself, as long as
whitespace is significant in some languages.  Fortunately, that's not
the case for Emacs Lisp or C.

Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 28 Oct 2022 23:52:11 -0700
> Cc: rms@gnu.org, joaotavora@gmail.com, emacs-devel@gnu.org, 
> 	stephen_leake@stephe-leake.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > They also make patches harder to review, sometimes much harder.
> 
> Fully agreed.  That's why I'd prefer it if people would make whitespace
> changes separately from functional changes.  Our current rule
> unfortunately seems to encourage putting it all in the same patch.

Having whitespace changes as part of code changes is inevitable, when
the changes modify indentation.  You cannot separate them.  We ask not
to make whitespace changes in places other than where the code is
being changed because that makes finding the "real" changes harder
than it has to be.

As for whitespace changes unrelated to code changes: we prefer not to
make them at all.

In any case, the suggestion to change our policy is a separate
discussion.  My intent was to point out that we do have a policy, and
to make sure people who read this list are aware of that policy.