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

[Ruijie Yu via "Emacs development discussions." <emacs-devel@gnu.org>]

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