Code quality of some -ts-mode major modes

Code quality of some -ts-mode major modes

[Philip Kaludercic]

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


Hi, I took a look at some of the new tree-sitter major modes and was
surprised at what I saw.  Without meaning to belittle anyone, there were
some basic "stylistic mistakes" that I wouldn't have expected to have
gotten merged.  I didn't look up the exact chronology, but it seems like
there has been a lot of uncritical copying between these files.

Take `yaml-ts-mode' for example.  I attach a diff with a few changes
that I would have expected to have been made before the file was merged:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 2642 bytes --]

diff --git a/lisp/textmodes/yaml-ts-mode.el b/lisp/textmodes/yaml-ts-mode.el
index dfa8d22fb34..43e8868fc44 100644
--- a/lisp/textmodes/yaml-ts-mode.el
+++ b/lisp/textmodes/yaml-ts-mode.el
@@ -2,10 +2,10 @@
 
 ;; Copyright (C) 2022-2023 Free Software Foundation, Inc.
 
-;; Author     : Randy Taylor <dev@rjt.dev>
-;; Maintainer : Randy Taylor <dev@rjt.dev>
-;; Created    : December 2022
-;; Keywords   : yaml languages tree-sitter
+;; Author: Randy Taylor <dev@rjt.dev>
+;; Maintainer: Randy Taylor <dev@rjt.dev>
+;; Created: December 2022
+;; Keywords: languages
 
 ;; This file is part of GNU Emacs.
 
@@ -23,15 +23,18 @@
 ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
 
 ;;; Commentary:
-;;
+
+;; This file provides basic YAML syntax highlighting using Tree
+;; Sitter.  To use the `yaml-ts-mode' major mode you will have to make
+;; sure that you have installed the appropriate grammar.
 
 ;;; Code:
 
 (require 'treesit)
 
-(declare-function treesit-parser-create "treesit.c")
+;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear necessary
 
-(defvar yaml-ts-mode--syntax-table
+(defvar yaml-ts-mode-syntax-table
   (let ((table (make-syntax-table)))
     (modify-syntax-entry ?#  "<"  table)
     (modify-syntax-entry ?\n ">"  table)
@@ -59,7 +62,8 @@ yaml-ts-mode--font-lock-settings
       (null_scalar)
       (reserved_directive)
       (tag_directive)
-      (yaml_directive)] @font-lock-constant-face)
+      (yaml_directive)]
+     @font-lock-constant-face)
 
    :language 'yaml
    :feature 'delimiter
@@ -83,7 +87,8 @@ yaml-ts-mode--font-lock-settings
    '([(block_scalar)
       (double_quote_scalar)
       (single_quote_scalar)
-      (string_scalar)] @font-lock-string-face)
+      (string_scalar)]
+     @font-lock-string-face)
 
    :language 'yaml
    :feature 'escape-sequence
@@ -120,10 +125,9 @@ yaml-ts-mode--font-lock-settings
 ;;;###autoload
 (define-derived-mode yaml-ts-mode text-mode "YAML"
   "Major mode for editing YAML, powered by tree-sitter."
-  :group 'yaml
-  :syntax-table yaml-ts-mode--syntax-table
+  ;; :group 'yaml ;; no such customisation group was defined?
 
-  (when (treesit-ready-p 'yaml)
+  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
     (treesit-parser-create 'yaml)
 
     ;; Comments.
@@ -143,9 +147,8 @@ yaml-ts-mode
 
     (treesit-major-mode-setup)))
 
-(if (treesit-ready-p 'yaml)
-    (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
+(when (treesit-ready-p 'yaml)
+  (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
 
 (provide 'yaml-ts-mode)
-
 ;;; yaml-ts-mode.el ends here

[-- Attachment #3: Type: text/plain, Size: 764 bytes --]


In particular: The lack of a commentary section or any
indication/pointer on how to install the grammar which is the necessary
prequisite for the mode to have any effect to begin with (my
understanding is that Emacs will not ship with these files, nor are any
distributions working on providing them, right?).  I considered
mentioning the new command `treesit-install-language-grammar', but that
seems pointless as long as `treesit-language-source-alist' is empty by
default.

My question: Would there be any objection from those involves with
tree-sitter against me making changes like the ones I gave above?
Maintaining some basic style in the core seems desirable to me, as we
have seen that these files often serve as a template for creating new
major modes.

Re: Code quality of some -ts-mode major modes

[Ruijie Yu via Emacs development discussions.]

Hello Philip,

Not a maintainer, but wanted to chime in to share some of my
observations and comments.

> -;; Author     : Randy Taylor <dev@rjt.dev>
> -;; Maintainer : Randy Taylor <dev@rjt.dev>
> -;; Created    : December 2022
> -;; Keywords   : yaml languages tree-sitter
> +;; Author: Randy Taylor <dev@rjt.dev>
> +;; Maintainer: Randy Taylor <dev@rjt.dev>
> +;; Created: December 2022
> +;; Keywords: languages

This seems to be mostly just personal preference on aesthetics (whether
the colons should be aligned with each other, or placed right after the
left side).

I also realize you have removed the keywords "yaml" and "tree-sitter",
why so?

> -;;
> +
> +;; This file provides basic YAML syntax highlighting using Tree
> +;; Sitter.  To use the `yaml-ts-mode' major mode you will have to make
> +;; sure that you have installed the appropriate grammar.

To me this does provide some useful commentary, so maybe this change is
justifiable.

>  (require 'treesit)
>
> -(declare-function treesit-parser-create "treesit.c")
> +;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear necessary

I noticed this portion as well as in c-ts-mode.el, where a bunch of
`declare-function''s follow `(require 'treesit)'.  Does it work if all
calls to `(require 'treesit)' are wrapped with `eval-and-compile', and
we remove all the `declare-function''s?  Or were these
`declare-functions' calls simply there for redundancy?

> -(defvar yaml-ts-mode--syntax-table
> +(defvar yaml-ts-mode-syntax-table

This might be justifiable on the basis that `define-derived-mode' uses
CHILD-syntax-table as the name of the default syntax table -- although
the original dev might have a different idea.

> -      (yaml_directive)] @font-lock-constant-face)
> +      (yaml_directive)]
> +     @font-lock-constant-face)
>
> -      (string_scalar)] @font-lock-string-face)
> +      (string_scalar)]
> +     @font-lock-string-face)

I guess you wanted to insert newlines there because you saw these in
`font-lock-warning-face' -- makes sense to me.

> -  (when (treesit-ready-p 'yaml)
> +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>      (treesit-parser-create 'yaml)

Raising a `user-error' would disallow the user from staying in the TS
mode (in this case, `yaml-ts-mode').  IIRC, someone said that a TS mode
should be roughly the same as `fundamental-mode' if the respective TS
grammar library is absent.  Not sure exactly, so let's wait for a
maintainer's response on that.

> -(if (treesit-ready-p 'yaml)
> -    (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
> +(when (treesit-ready-p 'yaml)
> +  (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))

Functionally, this doesn't change anything, because the following is the
definition of `when' in subr.el:

```emacs-lisp
(defmacro when (cond &rest body)
  "If COND yields non-nil, do BODY, else return nil.
When COND yields non-nil, eval BODY forms sequentially and return
value of last one, or nil if there are none."
  (declare (indent 1) (debug t))
  (if body
      (list 'if cond (cons 'progn body))
    (macroexp-warn-and-return (format-message "`when' with empty body")
                              cond '(empty-body when) t)))
```

As you can see, `when' simply reduces to `if' with an empty else
expression.  I have no comments on the difference in their indentation
styles though.

> In particular: The lack of a commentary section or any
> indication/pointer on how to install the grammar which is the necessary
> prequisite for the mode to have any effect to begin with (my
> understanding is that Emacs will not ship with these files, nor are any
> distributions working on providing them, right?).  I considered
> mentioning the new command `treesit-install-language-grammar', but that
> seems pointless as long as `treesit-language-source-alist' is empty by
> default.

I remember someone said in one of the Emacs MLs that a given TS mode
should mention against which grammar library it has been tested.  That
proposal seems to at least somewhat align with what you said here.

> [...]

--
Best,


RY

Re: Code quality of some -ts-mode major modes

[Eli Zaretskii]

> Cc: Yuan Fu <casouri@gmail.com>, emacs-devel@gnu.org
> Date: Fri, 17 Mar 2023 18:29:52 +0800
> From:  Ruijie Yu via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> >  (require 'treesit)
> >
> > -(declare-function treesit-parser-create "treesit.c")
> > +;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear necessary
> 
> I noticed this portion as well as in c-ts-mode.el, where a bunch of
> `declare-function''s follow `(require 'treesit)'.  Does it work if all
> calls to `(require 'treesit)' are wrapped with `eval-and-compile', and
> we remove all the `declare-function''s?

No, it doesn't.  The declare-function are about functions implemented
in treesit.c, not treesit.el, so loading the latter cannot possibly
fix the need for declare-function in these cases.

> Or were these `declare-functions' calls simply there for redundancy?

No, they are there to avoid byte-compiler warnings when building Emacs
without tree-sitter support (which is optional).

> > -  (when (treesit-ready-p 'yaml)
> > +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
> >      (treesit-parser-create 'yaml)
> 
> Raising a `user-error' would disallow the user from staying in the TS
> mode (in this case, `yaml-ts-mode').  IIRC, someone said that a TS mode
> should be roughly the same as `fundamental-mode' if the respective TS
> grammar library is absent.  Not sure exactly, so let's wait for a
> maintainer's response on that.

We _want_ this to signal an error so that the user is acutely aware
his/her system is not configured for these modes.

Re: Code quality of some -ts-mode major modes

[Philip Kaludercic]

Ruijie Yu <ruijie@netyu.xyz> writes:

> Hello Philip,
>
> Not a maintainer, but wanted to chime in to share some of my
> observations and comments.
>
>> -;; Author     : Randy Taylor <dev@rjt.dev>
>> -;; Maintainer : Randy Taylor <dev@rjt.dev>
>> -;; Created    : December 2022
>> -;; Keywords   : yaml languages tree-sitter
>> +;; Author: Randy Taylor <dev@rjt.dev>
>> +;; Maintainer: Randy Taylor <dev@rjt.dev>
>> +;; Created: December 2022
>> +;; Keywords: languages
>
> This seems to be mostly just personal preference on aesthetics (whether
> the colons should be aligned with each other, or placed right after the
> left side).

This is currently only the case in -ts-mode.el files, or at least when I
look up the regular expression ";; Author[[:space:]]+:", these are the
only files that appear.

> I also realize you have removed the keywords "yaml" and "tree-sitter",
> why so?

I was thinking of a comment in (elisp) Simple Packages,

           The ‘Keywords’ and ‘URL’ headers are optional, but recommended.  The
        command ‘describe-package’ uses these to add links to its output.  The
        ‘Keywords’ header should contain at least one standard keyword from the
        ‘finder-known-keywords’ list.

But I misremembered "at least one" as being "only".  Other than that,
the list should be comma separated.

>> -;;
>> +
>> +;; This file provides basic YAML syntax highlighting using Tree
>> +;; Sitter.  To use the `yaml-ts-mode' major mode you will have to make
>> +;; sure that you have installed the appropriate grammar.
>
> To me this does provide some useful commentary, so maybe this change is
> justifiable.
>
>>  (require 'treesit)
>>
>> -(declare-function treesit-parser-create "treesit.c")
>> +;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear necessary
>
> I noticed this portion as well as in c-ts-mode.el, where a bunch of
> `declare-function''s follow `(require 'treesit)'.  Does it work if all
> calls to `(require 'treesit)' are wrapped with `eval-and-compile', and
> we remove all the `declare-function''s?  Or were these
> `declare-functions' calls simply there for redundancy?

Eli explained this below.

>> -(defvar yaml-ts-mode--syntax-table
>> +(defvar yaml-ts-mode-syntax-table
>
> This might be justifiable on the basis that `define-derived-mode' uses
> CHILD-syntax-table as the name of the default syntax table -- although
> the original dev might have a different idea.
>
>> -      (yaml_directive)] @font-lock-constant-face)
>> +      (yaml_directive)]
>> +     @font-lock-constant-face)
>>
>> -      (string_scalar)] @font-lock-string-face)
>> +      (string_scalar)]
>> +     @font-lock-string-face)
>
> I guess you wanted to insert newlines there because you saw these in
> `font-lock-warning-face' -- makes sense to me.
>
>> -  (when (treesit-ready-p 'yaml)
>> +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>>      (treesit-parser-create 'yaml)
>
> Raising a `user-error' would disallow the user from staying in the TS
> mode (in this case, `yaml-ts-mode').  IIRC, someone said that a TS mode
> should be roughly the same as `fundamental-mode' if the respective TS
> grammar library is absent.  Not sure exactly, so let's wait for a
> maintainer's response on that.



>> -(if (treesit-ready-p 'yaml)
>> -    (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
>> +(when (treesit-ready-p 'yaml)
>> +  (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
>
> Functionally, this doesn't change anything, because the following is the
> definition of `when' in subr.el:
>
> ```emacs-lisp
> (defmacro when (cond &rest body)
>   "If COND yields non-nil, do BODY, else return nil.
> When COND yields non-nil, eval BODY forms sequentially and return
> value of last one, or nil if there are none."
>   (declare (indent 1) (debug t))
>   (if body
>       (list 'if cond (cons 'progn body))
>     (macroexp-warn-and-return (format-message "`when' with empty body")
>                               cond '(empty-body when) t)))
> ```
>
> As you can see, `when' simply reduces to `if' with an empty else
> expression.  I have no comments on the difference in their indentation
> styles though.

The rule-of-thumb that I go by is that `if' is used if you have two
cases you are interested in, especially if you are interested in the
return value, while `when' is more "imperative" in style and indicates
to the reader that the code is being executed for a side-effect.

>> In particular: The lack of a commentary section or any
>> indication/pointer on how to install the grammar which is the necessary
>> prequisite for the mode to have any effect to begin with (my
>> understanding is that Emacs will not ship with these files, nor are any
>> distributions working on providing them, right?).  I considered
>> mentioning the new command `treesit-install-language-grammar', but that
>> seems pointless as long as `treesit-language-source-alist' is empty by
>> default.
>
> I remember someone said in one of the Emacs MLs that a given TS mode
> should mention against which grammar library it has been tested.  That
> proposal seems to at least somewhat align with what you said here.

But at that point, why just "mention" them and not directly add the
grammar to `treesit-language-source-alist'?  I am not a fan of the
current implementation, in that it clones a git directory and
immediately throws it away, but at least it is convenient.  Or is there
some freedom issue here?

>> [...]
>
> --
> Best,
>
>
> RY

Eli Zaretskii <eliz@gnu.org> writes:

>> > -  (when (treesit-ready-p 'yaml)
>> > +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>> >      (treesit-parser-create 'yaml)
>> 
>> Raising a `user-error' would disallow the user from staying in the TS
>> mode (in this case, `yaml-ts-mode').  IIRC, someone said that a TS mode
>> should be roughly the same as `fundamental-mode' if the respective TS
>> grammar library is absent.  Not sure exactly, so let's wait for a
>> maintainer's response on that.
>
> We _want_ this to signal an error so that the user is acutely aware
> his/her system is not configured for these modes.

Your comment here confuses me?  

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Yuan Fu <casouri@gmail.com>
>> Date: Fri, 17 Mar 2023 10:08:52 +0000
>> 
>> Hi, I took a look at some of the new tree-sitter major modes and was
>> surprised at what I saw.  Without meaning to belittle anyone, there were
>> some basic "stylistic mistakes" that I wouldn't have expected to have
>> gotten merged.  I didn't look up the exact chronology, but it seems like
>> there has been a lot of uncritical copying between these files.
>
> These remarks are not helpful and should have been omitted from the
> message, IMNSHO.

My intention is just to clarify that these are not personal attacks on
any of the contributors or people who have merged the changes.

>> @@ -23,15 +23,18 @@
>>  ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
>>  
>>  ;;; Commentary:
>> -;;
>> +
>> +;; This file provides basic YAML syntax highlighting using Tree
>> +;; Sitter.  To use the `yaml-ts-mode' major mode you will have to make
>> +;; sure that you have installed the appropriate grammar.
>
> Adding helpful comments is always welcome, and shouldn't be
> controversial.  There's also no end to adding such helpful comments,
> so just feel free to add them when you think they could help.

1+

>>  ;;; Code:
>>  
>>  (require 'treesit)
>>  
>> -(declare-function treesit-parser-create "treesit.c")
>> +;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear necessary
>
> If the function is not used, removing the declare-function is OK.
>
>> @@ -120,10 +125,9 @@ yaml-ts-mode--font-lock-settings
>>  ;;;###autoload
>>  (define-derived-mode yaml-ts-mode text-mode "YAML"
>>    "Major mode for editing YAML, powered by tree-sitter."
>> -  :group 'yaml
>> -  :syntax-table yaml-ts-mode--syntax-table
>> +  ;; :group 'yaml ;; no such customisation group was defined?
>
> Should we add such a group?

Is it worth to add a group with no user options?

>> -  (when (treesit-ready-p 'yaml)
>> +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>>      (treesit-parser-create 'yaml)
>
> This is intentional, and I explained it many times.

The reason I am confused here is that -- unless invoked manually --
yaml-ts-mode will not be added to `auto-mode-alist' if (treesit-ready-p
'yaml) wouldn't already evaluate to a non-nil value.

>> In particular: The lack of a commentary section or any
>> indication/pointer on how to install the grammar which is the necessary
>> prequisite for the mode to have any effect to begin with (my
>> understanding is that Emacs will not ship with these files, nor are any
>> distributions working on providing them, right?).
>
> There's a description in NEWS.  But mentioning the specific grammar
> with which the mode was tested is useful anyway.

Where exactly is this description?  Considering this example, all I see
is

--8<---------------cut here---------------start------------->8---
+++
*** New major mode 'yaml-ts-mode'.
A major mode based on the tree-sitter library for editing files
written in YAML.
--8<---------------cut here---------------end--------------->8---

Where "the tree-sitter library" can be confusing, because if you look
around on the www, you will find [0], but that doesn't appear to be part
of the "official" Tree Sitter organisation [1].

I repeat my question from above, if we are ready to link to the
grammars, wouldn't it make sense to populate the variable
`treesit-language-source-alist' directly?

[0] https://github.com/ikatyang/tree-sitter-yaml
[1] https://github.com/tree-sitter

>> My question: Would there be any objection from those involves with
>> tree-sitter against me making changes like the ones I gave above?
>
> Please post the patches for review, but in general they are, of
> course, welcome.  These modes are very "young", so it doesn't surprise
> me there are some stylistic issues with them.  That said, not
> everything you see is such an issue, especially if you weren't
> involved in the relevant discussions.

OK, I'll try and do so.

>> Maintaining some basic style in the core seems desirable to me, as we
>> have seen that these files often serve as a template for creating new
>> major modes.
>
> You are preaching to the choir here.

Of course, which is why the state of these files was unexpected.

Re: Code quality of some -ts-mode major modes

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Yuan Fu <casouri@gmail.com>,  emacs-devel@gnu.org
> Date: Fri, 17 Mar 2023 12:37:40 +0000
> 
> >> -(if (treesit-ready-p 'yaml)
> >> -    (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
> >> +(when (treesit-ready-p 'yaml)
> >> +  (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
> >
> > Functionally, this doesn't change anything, because the following is the
> > definition of `when' in subr.el:
> >
> > ```emacs-lisp
> > (defmacro when (cond &rest body)
> >   "If COND yields non-nil, do BODY, else return nil.
> > When COND yields non-nil, eval BODY forms sequentially and return
> > value of last one, or nil if there are none."
> >   (declare (indent 1) (debug t))
> >   (if body
> >       (list 'if cond (cons 'progn body))
> >     (macroexp-warn-and-return (format-message "`when' with empty body")
> >                               cond '(empty-body when) t)))
> > ```
> >
> > As you can see, `when' simply reduces to `if' with an empty else
> > expression.  I have no comments on the difference in their indentation
> > styles though.
> 
> The rule-of-thumb that I go by is that `if' is used if you have two
> cases you are interested in, especially if you are interested in the
> return value, while `when' is more "imperative" in style and indicates
> to the reader that the code is being executed for a side-effect.

That is your personal preference.  Objectively, there's nothing wrong
with using 'if' that has no 'else' part.  So changing someone's code
to use 'when' where 'if' can do, or vice versa -- replacing 'when'
with a single sexp in the body with 'if' -- has no real justification.

> > I remember someone said in one of the Emacs MLs that a given TS mode
> > should mention against which grammar library it has been tested.  That
> > proposal seems to at least somewhat align with what you said here.
> 
> But at that point, why just "mention" them and not directly add the
> grammar to `treesit-language-source-alist'?

Because if we add that to the code, we will need to maintain that for
the observable future to be correct.  Comments, even if they are
outdated, don't need such level of maintenance.  Moreover, the fact
that a given grammar was used for testing doesn't mean another grammar
will not work as well.

> >> > -  (when (treesit-ready-p 'yaml)
> >> > +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
> >> >      (treesit-parser-create 'yaml)
> >> 
> >> Raising a `user-error' would disallow the user from staying in the TS
> >> mode (in this case, `yaml-ts-mode').  IIRC, someone said that a TS mode
> >> should be roughly the same as `fundamental-mode' if the respective TS
> >> grammar library is absent.  Not sure exactly, so let's wait for a
> >> maintainer's response on that.
> >
> > We _want_ this to signal an error so that the user is acutely aware
> > his/her system is not configured for these modes.
> 
> Your comment here confuses me?  

What is confusing?

Again, I explained the rationale many times here.  I can explain
again, but is that really necessary?

> >> Hi, I took a look at some of the new tree-sitter major modes and was
> >> surprised at what I saw.  Without meaning to belittle anyone, there were
> >> some basic "stylistic mistakes" that I wouldn't have expected to have
> >> gotten merged.  I didn't look up the exact chronology, but it seems like
> >> there has been a lot of uncritical copying between these files.
> >
> > These remarks are not helpful and should have been omitted from the
> > message, IMNSHO.
> 
> My intention is just to clarify that these are not personal attacks on
> any of the contributors or people who have merged the changes.

Unfortunately, they sound exactly that.

Please keep in mind that most of the code of these modes was written
by relative newcomers to Emacs development, who had to learn our
coding conventions and subtle aspects of Emacs in a very short time,
while producing code that is supposed to be stable and solid enough to
go into the upcoming release.  The challenge which they faced was so
tough that frankly I didn't believe they will be able to make it
happen.  To my surprise and admiration, they did, and did it with
flying colors.  The issues you mention are real, but they are minor.
We can and should fix them without trying to be too judgmental to
those who labored on the code under such tense requirements.

> >> @@ -120,10 +125,9 @@ yaml-ts-mode--font-lock-settings
> >>  ;;;###autoload
> >>  (define-derived-mode yaml-ts-mode text-mode "YAML"
> >>    "Major mode for editing YAML, powered by tree-sitter."
> >> -  :group 'yaml
> >> -  :syntax-table yaml-ts-mode--syntax-table
> >> +  ;; :group 'yaml ;; no such customisation group was defined?
> >
> > Should we add such a group?
> 
> Is it worth to add a group with no user options?

If it is likely we will want to add options in the near future, then
yes.  (I just asked a question, I don't have a firm opinion on this
matter, and will not object deleting the :group part if we decide a
new group is not justified.)

> >> -  (when (treesit-ready-p 'yaml)
> >> +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
> >>      (treesit-parser-create 'yaml)
> >
> > This is intentional, and I explained it many times.
> 
> The reason I am confused here is that -- unless invoked manually --
> yaml-ts-mode will not be added to `auto-mode-alist' if (treesit-ready-p
> 'yaml) wouldn't already evaluate to a non-nil value.

It will also work if the grammar library is installed and the package
is loaded, whether manually or not.  So I still don't think I
understand what confused you.

> >> In particular: The lack of a commentary section or any
> >> indication/pointer on how to install the grammar which is the necessary
> >> prequisite for the mode to have any effect to begin with (my
> >> understanding is that Emacs will not ship with these files, nor are any
> >> distributions working on providing them, right?).
> >
> > There's a description in NEWS.  But mentioning the specific grammar
> > with which the mode was tested is useful anyway.
> 
> Where exactly is this description?

At the beginning of NEWS, where we say that Emacs can be built with
the tree-sitter library.

> Considering this example, all I see is
> 
> --8<---------------cut here---------------start------------->8---
> +++
> *** New major mode 'yaml-ts-mode'.
> A major mode based on the tree-sitter library for editing files
> written in YAML.
> --8<---------------cut here---------------end--------------->8---
> 
> Where "the tree-sitter library" can be confusing, because if you look
> around on the www, you will find [0], but that doesn't appear to be part
> of the "official" Tree Sitter organisation [1].

The assumption is that people either read NEWS in its entirety, or at
least search it for "tree-sitter".  If they only read parts, then I'm
sure they will sometimes be confused.

> I repeat my question from above, if we are ready to link to the
> grammars, wouldn't it make sense to populate the variable
> `treesit-language-source-alist' directly?

No, I don't want to do that, see above for the reasons.  (We had this
discussion about 2 months ago, when the command was added to Emacs.)

> >> Maintaining some basic style in the core seems desirable to me, as we
> >> have seen that these files often serve as a template for creating new
> >> major modes.
> >
> > You are preaching to the choir here.
> 
> Of course, which is why the state of these files was unexpected.

Help in reviewing patches when they are posted is also very welcome.
It takes more than one pair of eyes to spot every bit that needs
attention.

Re: Code quality of some -ts-mode major modes

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Yuan Fu <casouri@gmail.com>,  emacs-devel@gnu.org
>> Date: Fri, 17 Mar 2023 12:37:40 +0000
>> 
>> >> -(if (treesit-ready-p 'yaml)
>> >> -    (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
>> >> +(when (treesit-ready-p 'yaml)
>> >> +  (add-to-list 'auto-mode-alist '("\\.ya?ml\\'" . yaml-ts-mode)))
>> >
>> > Functionally, this doesn't change anything, because the following is the
>> > definition of `when' in subr.el:
>> >
>> > ```emacs-lisp
>> > (defmacro when (cond &rest body)
>> >   "If COND yields non-nil, do BODY, else return nil.
>> > When COND yields non-nil, eval BODY forms sequentially and return
>> > value of last one, or nil if there are none."
>> >   (declare (indent 1) (debug t))
>> >   (if body
>> >       (list 'if cond (cons 'progn body))
>> >     (macroexp-warn-and-return (format-message "`when' with empty body")
>> >                               cond '(empty-body when) t)))
>> > ```
>> >
>> > As you can see, `when' simply reduces to `if' with an empty else
>> > expression.  I have no comments on the difference in their indentation
>> > styles though.
>> 
>> The rule-of-thumb that I go by is that `if' is used if you have two
>> cases you are interested in, especially if you are interested in the
>> return value, while `when' is more "imperative" in style and indicates
>> to the reader that the code is being executed for a side-effect.
>
> That is your personal preference.  Objectively, there's nothing wrong
> with using 'if' that has no 'else' part.  So changing someone's code
> to use 'when' where 'if' can do, or vice versa -- replacing 'when'
> with a single sexp in the body with 'if' -- has no real justification.

Technically no, but I do hope not to be mistaken that there is a
convention (along the lines I gave above) here that goes beyond just my
personal preference.  CLTL even says[p. 166]:

    As a matter of style, when is normally used to conditionally produce
    some side effects, and the value of the when form is normally not
    used. If the value is relevant, then it may be stylistically more
    appropriate to use and or if.

>> > I remember someone said in one of the Emacs MLs that a given TS mode
>> > should mention against which grammar library it has been tested.  That
>> > proposal seems to at least somewhat align with what you said here.
>> 
>> But at that point, why just "mention" them and not directly add the
>> grammar to `treesit-language-source-alist'?
>
> Because if we add that to the code, we will need to maintain that for
> the observable future to be correct.  Comments, even if they are
> outdated, don't need such level of maintenance.  

That could be resolved by either pinning a revision or instead of
cloning the repository to download a tarball of a tag.  In fact that
should make the system even more stable than the way I see it being
promoted around the web currently, that just maps languages to Git
repository URLs.

>                                                  Moreover, the fact
> that a given grammar was used for testing doesn't mean another grammar
> will not work as well.

I don't know of any language with multiple independent implementations
(that is out of ignorance, not omniscience).  For that reason I don't
know how they would behave or how robust the Emacs implementations are
when the behaviour changes?

>> >> > -  (when (treesit-ready-p 'yaml)
>> >> > +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>> >> >      (treesit-parser-create 'yaml)
>> >> 
>> >> Raising a `user-error' would disallow the user from staying in the TS
>> >> mode (in this case, `yaml-ts-mode').  IIRC, someone said that a TS mode
>> >> should be roughly the same as `fundamental-mode' if the respective TS
>> >> grammar library is absent.  Not sure exactly, so let's wait for a
>> >> maintainer's response on that.
>> >
>> > We _want_ this to signal an error so that the user is acutely aware
>> > his/her system is not configured for these modes.
>> 
>> Your comment here confuses me?  
>
> What is confusing?
>
> Again, I explained the rationale many times here.  I can explain
> again, but is that really necessary?

You had previously said that you are opposed to raising an error (or am
I mistaken?), while the above comment says "we want this to signal and
error".

>> >> Hi, I took a look at some of the new tree-sitter major modes and was
>> >> surprised at what I saw.  Without meaning to belittle anyone, there were
>> >> some basic "stylistic mistakes" that I wouldn't have expected to have
>> >> gotten merged.  I didn't look up the exact chronology, but it seems like
>> >> there has been a lot of uncritical copying between these files.
>> >
>> > These remarks are not helpful and should have been omitted from the
>> > message, IMNSHO.
>> 
>> My intention is just to clarify that these are not personal attacks on
>> any of the contributors or people who have merged the changes.
>
> Unfortunately, they sound exactly that.

I know, but as this was not intended I wanted to clarify that this was
not the case.  Either way it is not important -- as most of these issues
are non-technical it is just difficult to object with reference to some
objective point.

> Please keep in mind that most of the code of these modes was written
> by relative newcomers to Emacs development, who had to learn our
> coding conventions and subtle aspects of Emacs in a very short time,
> while producing code that is supposed to be stable and solid enough to
> go into the upcoming release.  The challenge which they faced was so
> tough that frankly I didn't believe they will be able to make it
> happen.  To my surprise and admiration, they did, and did it with
> flying colors.  The issues you mention are real, but they are minor.
> We can and should fix them without trying to be too judgmental to
> those who labored on the code under such tense requirements.

Sure, I just noticed how these "issues" (if we even want to call them
that) were spreading, which I argue is the greater "issue".

>> >> @@ -120,10 +125,9 @@ yaml-ts-mode--font-lock-settings
>> >>  ;;;###autoload
>> >>  (define-derived-mode yaml-ts-mode text-mode "YAML"
>> >>    "Major mode for editing YAML, powered by tree-sitter."
>> >> -  :group 'yaml
>> >> -  :syntax-table yaml-ts-mode--syntax-table
>> >> +  ;; :group 'yaml ;; no such customisation group was defined?
>> >
>> > Should we add such a group?
>> 
>> Is it worth to add a group with no user options?
>
> If it is likely we will want to add options in the near future, then
> yes.  (I just asked a question, I don't have a firm opinion on this
> matter, and will not object deleting the :group part if we decide a
> new group is not justified.)

Neither to I have a firm opinion, except that a broken reference should
be avoided.

>> >> -  (when (treesit-ready-p 'yaml)
>> >> +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>> >>      (treesit-parser-create 'yaml)
>> >
>> > This is intentional, and I explained it many times.
>> 
>> The reason I am confused here is that -- unless invoked manually --
>> yaml-ts-mode will not be added to `auto-mode-alist' if (treesit-ready-p
>> 'yaml) wouldn't already evaluate to a non-nil value.
>
> It will also work if the grammar library is installed and the package
> is loaded, whether manually or not.  So I still don't think I
> understand what confused you.

I must have misunderstood something completely, because I can't even
parse your response.  It is probably best to set this aside for now.
I'll bring it up if I at some later point have a better grasp on the
details and still believe this to be an issue.

>> >> In particular: The lack of a commentary section or any
>> >> indication/pointer on how to install the grammar which is the necessary
>> >> prequisite for the mode to have any effect to begin with (my
>> >> understanding is that Emacs will not ship with these files, nor are any
>> >> distributions working on providing them, right?).
>> >
>> > There's a description in NEWS.  But mentioning the specific grammar
>> > with which the mode was tested is useful anyway.
>> 
>> Where exactly is this description?
>
> At the beginning of NEWS, where we say that Emacs can be built with
> the tree-sitter library.

OK, missed that.

>> Considering this example, all I see is
>> 
>> --8<---------------cut here---------------start------------->8---
>> +++
>> *** New major mode 'yaml-ts-mode'.
>> A major mode based on the tree-sitter library for editing files
>> written in YAML.
>> --8<---------------cut here---------------end--------------->8---
>> 
>> Where "the tree-sitter library" can be confusing, because if you look
>> around on the www, you will find [0], but that doesn't appear to be part
>> of the "official" Tree Sitter organisation [1].
>
> The assumption is that people either read NEWS in its entirety, or at
> least search it for "tree-sitter".  If they only read parts, then I'm
> sure they will sometimes be confused.

Fair point.

>> I repeat my question from above, if we are ready to link to the
>> grammars, wouldn't it make sense to populate the variable
>> `treesit-language-source-alist' directly?
>
> No, I don't want to do that, see above for the reasons.  (We had this
> discussion about 2 months ago, when the command was added to Emacs.)

I know but I didn't think a satisfying conclusion was found, and I
couldn't continue the discussion back then due to time constraints.

>> >> Maintaining some basic style in the core seems desirable to me, as we
>> >> have seen that these files often serve as a template for creating new
>> >> major modes.
>> >
>> > You are preaching to the choir here.
>> 
>> Of course, which is why the state of these files was unexpected.
>
> Help in reviewing patches when they are posted is also very welcome.
> It takes more than one pair of eyes to spot every bit that needs
> attention.

I'll try and do so when I notice one.  I have also been sketching out
support for a markdown-ts-mode to better understand the how tree-sitter
works, which could help.

-- 
Philip Kaludercic

Re: Code quality of some -ts-mode major modes

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: ruijie@netyu.xyz,  casouri@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 17 Mar 2023 15:20:49 +0000
> 
> >> The rule-of-thumb that I go by is that `if' is used if you have two
> >> cases you are interested in, especially if you are interested in the
> >> return value, while `when' is more "imperative" in style and indicates
> >> to the reader that the code is being executed for a side-effect.
> >
> > That is your personal preference.  Objectively, there's nothing wrong
> > with using 'if' that has no 'else' part.  So changing someone's code
> > to use 'when' where 'if' can do, or vice versa -- replacing 'when'
> > with a single sexp in the body with 'if' -- has no real justification.
> 
> Technically no, but I do hope not to be mistaken that there is a
> convention (along the lines I gave above) here that goes beyond just my
> personal preference.  CLTL even says[p. 166]:

It is fine for you to prefer this convention, but we don't mandate it
in Emacs.

> > Because if we add that to the code, we will need to maintain that for
> > the observable future to be correct.  Comments, even if they are
> > outdated, don't need such level of maintenance.  
> 
> That could be resolved by either pinning a revision or instead of
> cloning the repository to download a tarball of a tag.  In fact that
> should make the system even more stable than the way I see it being
> promoted around the web currently, that just maps languages to Git
> repository URLs.

It can be resolved in more than one way, but all of them mean
additional maintenance burden, so I don't think we should undertake
that.

> >                                                  Moreover, the fact
> > that a given grammar was used for testing doesn't mean another grammar
> > will not work as well.
> 
> I don't know of any language with multiple independent implementations

I do.  Indeed, most have just one.  But not all.

> > Again, I explained the rationale many times here.  I can explain
> > again, but is that really necessary?
> 
> You had previously said that you are opposed to raising an error (or am
> I mistaken?), while the above comment says "we want this to signal and
> error".

No you are mistaken.  We do want this to signal an error if
tree-sitter is not compiled in or the grammar is not available.

> > Help in reviewing patches when they are posted is also very welcome.
> > It takes more than one pair of eyes to spot every bit that needs
> > attention.
> 
> I'll try and do so when I notice one.  I have also been sketching out
> support for a markdown-ts-mode to better understand the how tree-sitter
> works, which could help.

TIA.

Re: Code quality of some -ts-mode major modes

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: ruijie@netyu.xyz,  casouri@gmail.com,  emacs-devel@gnu.org
>> Date: Fri, 17 Mar 2023 15:20:49 +0000
>> 
>> >> The rule-of-thumb that I go by is that `if' is used if you have two
>> >> cases you are interested in, especially if you are interested in the
>> >> return value, while `when' is more "imperative" in style and indicates
>> >> to the reader that the code is being executed for a side-effect.
>> >
>> > That is your personal preference.  Objectively, there's nothing wrong
>> > with using 'if' that has no 'else' part.  So changing someone's code
>> > to use 'when' where 'if' can do, or vice versa -- replacing 'when'
>> > with a single sexp in the body with 'if' -- has no real justification.
>> 
>> Technically no, but I do hope not to be mistaken that there is a
>> convention (along the lines I gave above) here that goes beyond just my
>> personal preference.  CLTL even says[p. 166]:
>
> It is fine for you to prefer this convention, but we don't mandate it
> in Emacs.

Ok, in that case I was mistaken.  I have just seen a number of commits
that either just or also made this change, making it seem like
established knowledge.

>> > Because if we add that to the code, we will need to maintain that for
>> > the observable future to be correct.  Comments, even if they are
>> > outdated, don't need such level of maintenance.  
>> 
>> That could be resolved by either pinning a revision or instead of
>> cloning the repository to download a tarball of a tag.  In fact that
>> should make the system even more stable than the way I see it being
>> promoted around the web currently, that just maps languages to Git
>> repository URLs.
>
> It can be resolved in more than one way, but all of them mean
> additional maintenance burden, so I don't think we should undertake
> that.

If you think so, but at least we agree that one reference to a grammar
would be helpful.

>> >                                                  Moreover, the fact
>> > that a given grammar was used for testing doesn't mean another grammar
>> > will not work as well.
>> 
>> I don't know of any language with multiple independent implementations
>
> I do.  Indeed, most have just one.  But not all.

Could you give me an example that I could try out?

>> > Again, I explained the rationale many times here.  I can explain
>> > again, but is that really necessary?
>> 
>> You had previously said that you are opposed to raising an error (or am
>> I mistaken?), while the above comment says "we want this to signal and
>> error".
>
> No you are mistaken.  We do want this to signal an error if
> tree-sitter is not compiled in or the grammar is not available.

But it doesn't, or am I completely oblivious that

  (treesit-ready-p 'something-i-just-made-up)

just displays a warning?  The way I am currently reading/seeing the code
is that if tree-sitter and the grammar are available, then it will be
initialised but otherwise we just get a warning and a pretty plain mode.

>> > Help in reviewing patches when they are posted is also very welcome.
>> > It takes more than one pair of eyes to spot every bit that needs
>> > attention.
>> 
>> I'll try and do so when I notice one.  I have also been sketching out
>> support for a markdown-ts-mode to better understand the how tree-sitter
>> works, which could help.
>
> TIA.

-- 
Philip Kaludercic

Re: Code quality of some -ts-mode major modes

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: ruijie@netyu.xyz,  casouri@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 17 Mar 2023 15:49:15 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I don't know of any language with multiple independent implementations
> >
> > I do.  Indeed, most have just one.  But not all.
> 
> Could you give me an example that I could try out?

One example I know of is sql.  I think there's at least one other, but
I don't remember which one.

> > No you are mistaken.  We do want this to signal an error if
> > tree-sitter is not compiled in or the grammar is not available.
> 
> But it doesn't, or am I completely oblivious that
> 
>   (treesit-ready-p 'something-i-just-made-up)
> 
> just displays a warning?

I'm not sure I understand the question.  Are you saying that the above
is silent, or are you saying that it pops up a warning instead of
signaling an error?  If the latter, this is the intended behavior;
sorry for confusingly saying it should error out.

Re: Code quality of some -ts-mode major modes

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: ruijie@netyu.xyz,  casouri@gmail.com,  emacs-devel@gnu.org
>> Date: Fri, 17 Mar 2023 15:49:15 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> I don't know of any language with multiple independent implementations
>> >
>> > I do.  Indeed, most have just one.  But not all.
>> 
>> Could you give me an example that I could try out?
>
> One example I know of is sql.  I think there's at least one other, but
> I don't remember which one.

For posterity, I'll just mentioned that I managed to find two:

- https://github.com/DerekStride/tree-sitter-sql ("general/permissive")
- https://github.com/m-novikov/tree-sitter-sql ("focus on postgresql")

>> > No you are mistaken.  We do want this to signal an error if
>> > tree-sitter is not compiled in or the grammar is not available.
>> 
>> But it doesn't, or am I completely oblivious that
>> 
>>   (treesit-ready-p 'something-i-just-made-up)
>> 
>> just displays a warning?
>
> I'm not sure I understand the question.  Are you saying that the above
> is silent, or are you saying that it pops up a warning instead of
> signaling an error?  If the latter, this is the intended behavior;
> sorry for confusingly saying it should error out.

The latter -- that also explains my confusion.

Re: Code quality of some -ts-mode major modes

[Dmitry Gutov]

On 17/03/2023 15:54, Eli Zaretskii wrote:
>> I repeat my question from above, if we are ready to link to the
>> grammars, wouldn't it make sense to populate the variable
>> `treesit-language-source-alist' directly?
> No, I don't want to do that, see above for the reasons.  (We had this
> discussion about 2 months ago, when the command was added to Emacs.)

FWIW, I had no problems with that conclusion, but then I noticed that we 
keep a separate list of sources inside admin/notes/tree-sitter, which 
basically contains the same info, as well as all 6 known exceptions to 
the general scenario (where in the usual case we can determine 
everything just from the name of the language).

If we're going to keep it updated (and we apparently are), why not move 
that info to treesit-language-source-alist instead.

We can make it a defcustom, to emphasize that people should update it 
whenever they see the data is old.

Re: Code quality of some -ts-mode major modes

[Eli Zaretskii]

> Date: Fri, 17 Mar 2023 23:45:00 +0200
> Cc: ruijie@netyu.xyz, casouri@gmail.com, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 17/03/2023 15:54, Eli Zaretskii wrote:
> >> I repeat my question from above, if we are ready to link to the
> >> grammars, wouldn't it make sense to populate the variable
> >> `treesit-language-source-alist' directly?
> > No, I don't want to do that, see above for the reasons.  (We had this
> > discussion about 2 months ago, when the command was added to Emacs.)
> 
> FWIW, I had no problems with that conclusion, but then I noticed that we 
> keep a separate list of sources inside admin/notes/tree-sitter, which 
> basically contains the same info, as well as all 6 known exceptions to 
> the general scenario (where in the usual case we can determine 
> everything just from the name of the language).
> 
> If we're going to keep it updated (and we apparently are), why not move 
> that info to treesit-language-source-alist instead.

My original plan was to remove that script from admin/notes before
releasing Emacs 29.  I decided to wait for a while, but I definitely
don't want to commit ourselves to maintaining the script in the future
versions.  The information is just one Internet search away, usually
the first hit is the one you want.

More generally, the tree-sitter stuff seems to be just starting to get
noticed by distros, and it is too early to make any firm decisions
related to availability of information and libraries.  We already did
in this case more than we usually do for optional features, and I
think it should be enough for now.  Further efforts that increase our
maintenance burden should wait until it is clear they are necessary.

Re: Code quality of some -ts-mode major modes

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Yuan Fu <casouri@gmail.com>
> Date: Fri, 17 Mar 2023 10:08:52 +0000
> 
> Hi, I took a look at some of the new tree-sitter major modes and was
> surprised at what I saw.  Without meaning to belittle anyone, there were
> some basic "stylistic mistakes" that I wouldn't have expected to have
> gotten merged.  I didn't look up the exact chronology, but it seems like
> there has been a lot of uncritical copying between these files.

These remarks are not helpful and should have been omitted from the
message, IMNSHO.

> @@ -23,15 +23,18 @@
>  ;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
>  
>  ;;; Commentary:
> -;;
> +
> +;; This file provides basic YAML syntax highlighting using Tree
> +;; Sitter.  To use the `yaml-ts-mode' major mode you will have to make
> +;; sure that you have installed the appropriate grammar.

Adding helpful comments is always welcome, and shouldn't be
controversial.  There's also no end to adding such helpful comments,
so just feel free to add them when you think they could help.

>  ;;; Code:
>  
>  (require 'treesit)
>  
> -(declare-function treesit-parser-create "treesit.c")
> +;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear necessary

If the function is not used, removing the declare-function is OK.

> @@ -120,10 +125,9 @@ yaml-ts-mode--font-lock-settings
>  ;;;###autoload
>  (define-derived-mode yaml-ts-mode text-mode "YAML"
>    "Major mode for editing YAML, powered by tree-sitter."
> -  :group 'yaml
> -  :syntax-table yaml-ts-mode--syntax-table
> +  ;; :group 'yaml ;; no such customisation group was defined?

Should we add such a group?

> -  (when (treesit-ready-p 'yaml)
> +  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
>      (treesit-parser-create 'yaml)

This is intentional, and I explained it many times.

> In particular: The lack of a commentary section or any
> indication/pointer on how to install the grammar which is the necessary
> prequisite for the mode to have any effect to begin with (my
> understanding is that Emacs will not ship with these files, nor are any
> distributions working on providing them, right?).

There's a description in NEWS.  But mentioning the specific grammar
with which the mode was tested is useful anyway.

> My question: Would there be any objection from those involves with
> tree-sitter against me making changes like the ones I gave above?

Please post the patches for review, but in general they are, of
course, welcome.  These modes are very "young", so it doesn't surprise
me there are some stylistic issues with them.  That said, not
everything you see is such an issue, especially if you weren't
involved in the relevant discussions.

> Maintaining some basic style in the core seems desirable to me, as we
> have seen that these files often serve as a template for creating new
> major modes.

You are preaching to the choir here.