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