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.