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.