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.