Re: emacs-29 56cd810b9d1: Don’t signal warning when loading go-ts-mode.el without grammar

Re: emacs-29 56cd810b9d1: Don’t signal warning when loading go-ts-mode.el without grammar

[Eli Zaretskii]

> branch: emacs-29
> commit 56cd810b9d1a4d537bee5a2fd954d6e0d346631a
> Author: Yuan Fu <casouri@gmail.com>
> Commit: Yuan Fu <casouri@gmail.com>
> 
>     Don’t signal warning when loading go-ts-mode.el without grammar
>     
>     * lisp/progmodes/go-ts-mode.el: Add a QUIET flag to the call of
>     treesit-ready-p, so that it doesn't signal a warning if
>     go-mod (tree-sitter grammar) is not available.
> ---
>  lisp/progmodes/go-ts-mode.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
> index e8f93d14744..6043c86ac93 100644
> --- a/lisp/progmodes/go-ts-mode.el
> +++ b/lisp/progmodes/go-ts-mode.el
> @@ -393,7 +393,7 @@ what the parent of the node would be if it were a node."
>  
>      (treesit-major-mode-setup)))
>  
> -(if (treesit-ready-p 'gomod)
> +(if (treesit-ready-p 'gomod t)
>      (add-to-list 'auto-mode-alist '("/go\\.mod\\'" . go-mod-ts-mode)))
>  
>  (provide 'go-ts-mode)

Yuan, why was this change made?  The fact that loading go-ts-mode.el
without a grammar library causes a warning is the intended behavior:
users who don't have tree-sitter setup for Go shouldn't load this
mode, and if they do, they should know it failed, and why.  The above
change makes it fail silently, which is not a Good Thing.

What was the problem you were trying to solve?

The subsequent change, viz.:

> branch: emacs-29
> commit 59365f928565f1be551b1697b9246b00cb87a9b7
> Author: Yuan Fu <casouri@gmail.com>
> Commit: Yuan Fu <casouri@gmail.com>
> 
>     * lisp/progmodes/go-ts-mode.el: Use treesit-language-available-p.
>     
>     treesit-ready-p does more checks than language grammar availability.
> ---
>  lisp/progmodes/go-ts-mode.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
> index 6043c86ac93..fbe085a2c6c 100644
> --- a/lisp/progmodes/go-ts-mode.el
> +++ b/lisp/progmodes/go-ts-mode.el
> @@ -393,7 +393,7 @@ what the parent of the node would be if it were a node."
>  
>      (treesit-major-mode-setup)))
>  
> -(if (treesit-ready-p 'gomod t)
> +(if (treesit-language-available-p 'gomod)
>      (add-to-list 'auto-mode-alist '("/go\\.mod\\'" . go-mod-ts-mode)))
>  
>  (provide 'go-ts-mode)

made this even worse: now we have a byte-compilation warning in a
build without tree-sitter, and this code will signal a generic error
(whose text is not very helpful to users) if the file is loaded in a
build without tree-sitter.  This is all against the intended behavior
of these modes.

Again, what problem were you trying to fix with that change?

Re: emacs-29 56cd810b9d1: Don’t signal warning when loading go-ts-mode.el without grammar

[Yuan Fu]

> 
> Yuan, why was this change made?  The fact that loading go-ts-mode.el
> without a grammar library causes a warning is the intended behavior:
> users who don't have tree-sitter setup for Go shouldn't load this
> mode, and if they do, they should know it failed, and why.  The above
> change makes it fail silently, which is not a Good Thing.

I thought that we’d want the warning to appear when user calls go-mod-ts-mode or open a file that uses that mode, rather than at load time. What I had in mind is when someone only install tree-sitter-go and want to edit some Go file, and they get this warning of go-mod being not available. This is what I encountered which prompted this change. I didn’t know that was intentional; since it’s intentional, please revert it (maybe you already have), and sorry for the hiccup :-)

> made this even worse: now we have a byte-compilation warning in a
> build without tree-sitter, and this code will signal a generic error
> (whose text is not very helpful to users) if the file is loaded in a
> build without tree-sitter.  This is all against the intended behavior
> of these modes.
> 
> Again, what problem were you trying to fix with that change?

Treesit-ready-p is intended to be used for major modes, so it includes checks for current buffer’s size, which doesn’t make much sense when called when loading the file. But if you intentionally used that function for the warning, I guess there is no harm using it this way, as the default size threshold is rather large.

Yuan

Re: emacs-29 56cd810b9d1: Don’t signal warning when loading go-ts-mode.el without grammar

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Thu, 2 Mar 2023 19:05:27 -0800
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > 
> > Yuan, why was this change made?  The fact that loading go-ts-mode.el
> > without a grammar library causes a warning is the intended behavior:
> > users who don't have tree-sitter setup for Go shouldn't load this
> > mode, and if they do, they should know it failed, and why.  The above
> > change makes it fail silently, which is not a Good Thing.
> 
> I thought that we’d want the warning to appear when user calls go-mod-ts-mode or open a file that uses that mode, rather than at load time. What I had in mind is when someone only install tree-sitter-go and want to edit some Go file, and they get this warning of go-mod being not available. This is what I encountered which prompted this change. I didn’t know that was intentional; since it’s intentional, please revert it (maybe you already have), and sorry for the hiccup :-)

This will happen with any *-ts-mode.el file that defines more than one
mode for more than one grammar.  The only way of avoiding this is to
separate the modes, and we decided not to do that, presumably for good
reasons.  So the result is that users who want to edit Go files must
also install the grammar for go.mod files (and similarly with C and
C++).  The root cause is that when the file is loaded, we have no way
of knowing for which of the two modes it was loaded.  This is a
disadvantage of this arrangement, but as long as we keep these modes
together on a single file, I don't see how we can avoid that.

So please revert those changes, as they go against what is planned for
Emacs 29.

> > made this even worse: now we have a byte-compilation warning in a
> > build without tree-sitter, and this code will signal a generic error
> > (whose text is not very helpful to users) if the file is loaded in a
> > build without tree-sitter.  This is all against the intended behavior
> > of these modes.
> > 
> > Again, what problem were you trying to fix with that change?
> 
> Treesit-ready-p is intended to be used for major modes, so it includes checks for current buffer’s size, which doesn’t make much sense when called when loading the file. But if you intentionally used that function for the warning, I guess there is no harm using it this way, as the default size threshold is rather large.

If we want to avoid the buffer size text in some cases, we can add an
optional argument to treesit-ready-p, and use it in those places.
(And I agree that the test is probably harmless anyway.)

Re: emacs-29 56cd810b9d1: Don’t signal warning when loading go-ts-mode.el without grammar

[Yuan Fu]

> On Mar 2, 2023, at 11:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Thu, 2 Mar 2023 19:05:27 -0800
>> Cc: Emacs developers <emacs-devel@gnu.org>
>> 
>>> 
>>> Yuan, why was this change made?  The fact that loading go-ts-mode.el
>>> without a grammar library causes a warning is the intended behavior:
>>> users who don't have tree-sitter setup for Go shouldn't load this
>>> mode, and if they do, they should know it failed, and why.  The above
>>> change makes it fail silently, which is not a Good Thing.
>> 
>> I thought that we’d want the warning to appear when user calls go-mod-ts-mode or open a file that uses that mode, rather than at load time. What I had in mind is when someone only install tree-sitter-go and want to edit some Go file, and they get this warning of go-mod being not available. This is what I encountered which prompted this change. I didn’t know that was intentional; since it’s intentional, please revert it (maybe you already have), and sorry for the hiccup :-)
> 
> This will happen with any *-ts-mode.el file that defines more than one
> mode for more than one grammar.  The only way of avoiding this is to
> separate the modes, and we decided not to do that, presumably for good
> reasons.  So the result is that users who want to edit Go files must
> also install the grammar for go.mod files (and similarly with C and
> C++).  The root cause is that when the file is loaded, we have no way
> of knowing for which of the two modes it was loaded.  This is a
> disadvantage of this arrangement, but as long as we keep these modes
> together on a single file, I don't see how we can avoid that.
> 
> So please revert those changes, as they go against what is planned for
> Emacs 29.
> 
>>> made this even worse: now we have a byte-compilation warning in a
>>> build without tree-sitter, and this code will signal a generic error
>>> (whose text is not very helpful to users) if the file is loaded in a
>>> build without tree-sitter.  This is all against the intended behavior
>>> of these modes.
>>> 
>>> Again, what problem were you trying to fix with that change?
>> 
>> Treesit-ready-p is intended to be used for major modes, so it includes checks for current buffer’s size, which doesn’t make much sense when called when loading the file. But if you intentionally used that function for the warning, I guess there is no harm using it this way, as the default size threshold is rather large.
> 
> If we want to avoid the buffer size text in some cases, we can add an
> optional argument to treesit-ready-p, and use it in those places.
> (And I agree that the test is probably harmless anyway.)

I’ve reverted the change.

Yuan

Re: emacs-29 56cd810b9d1: Don’t signal warning when loading go-ts-mode.el without grammar

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 3 Mar 2023 14:06:03 -0800
> Cc: emacs-devel@gnu.org
> 
> > So please revert those changes, as they go against what is planned for
> > Emacs 29.
> > 
> >>> made this even worse: now we have a byte-compilation warning in a
> >>> build without tree-sitter, and this code will signal a generic error
> >>> (whose text is not very helpful to users) if the file is loaded in a
> >>> build without tree-sitter.  This is all against the intended behavior
> >>> of these modes.
> >>> 
> >>> Again, what problem were you trying to fix with that change?
> >> 
> >> Treesit-ready-p is intended to be used for major modes, so it includes checks for current buffer’s size, which doesn’t make much sense when called when loading the file. But if you intentionally used that function for the warning, I guess there is no harm using it this way, as the default size threshold is rather large.
> > 
> > If we want to avoid the buffer size text in some cases, we can add an
> > optional argument to treesit-ready-p, and use it in those places.
> > (And I agree that the test is probably harmless anyway.)
> 
> I’ve reverted the change.

Thanks.