* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list [not found] ` <20221025091717.DD9A3C0E4BF@vcs2.savannah.gnu.org> @ 2022-10-25 9:29 ` João Távora 2022-10-25 9:35 ` João Távora 2022-10-27 20:13 ` Richard Stallman 0 siblings, 2 replies; 10+ messages in thread From: João Távora @ 2022-10-25 9:29 UTC (permalink / raw) To: emacs-devel, Stephen Leake [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-25 9:29 ` master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list João Távora @ 2022-10-25 9:35 ` João Távora 2022-10-27 20:13 ` Richard Stallman 1 sibling, 0 replies; 10+ messages in thread From: João Távora @ 2022-10-25 9:35 UTC (permalink / raw) To: emacs-devel, Stephen Leake [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-25 9:29 ` master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list João Távora 2022-10-25 9:35 ` João Távora @ 2022-10-27 20:13 ` Richard Stallman 2022-10-28 5:42 ` Eli Zaretskii 1 sibling, 1 reply; 10+ messages in thread From: Richard Stallman @ 2022-10-27 20:13 UTC (permalink / raw) To: João Távora; +Cc: emacs-devel, stephen_leake [[[ 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) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-27 20:13 ` Richard Stallman @ 2022-10-28 5:42 ` Eli Zaretskii 2022-10-28 8:32 ` João Távora 2022-10-28 19:45 ` Stefan Kangas 0 siblings, 2 replies; 10+ messages in thread From: Eli Zaretskii @ 2022-10-28 5:42 UTC (permalink / raw) To: rms; +Cc: joaotavora, emacs-devel, stephen_leake > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-28 5:42 ` Eli Zaretskii @ 2022-10-28 8:32 ` João Távora 2022-10-28 11:30 ` Eli Zaretskii 2022-10-28 19:45 ` Stefan Kangas 1 sibling, 1 reply; 10+ messages in thread From: João Távora @ 2022-10-28 8:32 UTC (permalink / raw) To: Eli Zaretskii; +Cc: rms, emacs-devel, stephen_leake [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-28 8:32 ` João Távora @ 2022-10-28 11:30 ` Eli Zaretskii 0 siblings, 0 replies; 10+ messages in thread From: Eli Zaretskii @ 2022-10-28 11:30 UTC (permalink / raw) To: João Távora; +Cc: rms, emacs-devel, stephen_leake > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-28 5:42 ` Eli Zaretskii 2022-10-28 8:32 ` João Távora @ 2022-10-28 19:45 ` Stefan Kangas 2022-10-29 5:56 ` Eli Zaretskii 1 sibling, 1 reply; 10+ messages in thread From: Stefan Kangas @ 2022-10-28 19:45 UTC (permalink / raw) To: Eli Zaretskii, rms; +Cc: joaotavora, emacs-devel, stephen_leake 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-28 19:45 ` Stefan Kangas @ 2022-10-29 5:56 ` Eli Zaretskii 2022-10-29 6:52 ` Stefan Kangas 0 siblings, 1 reply; 10+ messages in thread From: Eli Zaretskii @ 2022-10-29 5:56 UTC (permalink / raw) To: Stefan Kangas; +Cc: rms, joaotavora, emacs-devel, stephen_leake > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-29 5:56 ` Eli Zaretskii @ 2022-10-29 6:52 ` Stefan Kangas 2022-10-29 7:24 ` Eli Zaretskii 0 siblings, 1 reply; 10+ messages in thread From: Stefan Kangas @ 2022-10-29 6:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: rms, joaotavora, emacs-devel, stephen_leake 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list 2022-10-29 6:52 ` Stefan Kangas @ 2022-10-29 7:24 ` Eli Zaretskii 0 siblings, 0 replies; 10+ messages in thread From: Eli Zaretskii @ 2022-10-29 7:24 UTC (permalink / raw) To: Stefan Kangas; +Cc: rms, joaotavora, emacs-devel, stephen_leake > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-10-29 7:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <166668943749.31970.9379739764487638921@vcs2.savannah.gnu.org> [not found] ` <20221025091717.DD9A3C0E4BF@vcs2.savannah.gnu.org> 2022-10-25 9:29 ` master 31945b6c3f: * lisp/progmodes/eglot.el (eglot): Ensure managed-major-mode is a list João Távora 2022-10-25 9:35 ` João Távora 2022-10-27 20:13 ` Richard Stallman 2022-10-28 5:42 ` Eli Zaretskii 2022-10-28 8:32 ` João Távora 2022-10-28 11:30 ` Eli Zaretskii 2022-10-28 19:45 ` Stefan Kangas 2022-10-29 5:56 ` Eli Zaretskii 2022-10-29 6:52 ` Stefan Kangas 2022-10-29 7:24 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).