From: Philip Kaludercic <philipk@posteo.net>
To: Ruijie Yu <ruijie@netyu.xyz>
Cc: Yuan Fu <casouri@gmail.com>, emacs-devel@gnu.org
Subject: Re: Code quality of some -ts-mode major modes
Date: Fri, 17 Mar 2023 12:37:40 +0000 [thread overview]
Message-ID: <87bkkrft9n.fsf@posteo.net> (raw)
In-Reply-To: <sdvv8izws8x.fsf@fw.net.yu> (Ruijie Yu's message of "Fri, 17 Mar 2023 18:29:52 +0800")
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.
next prev parent reply other threads:[~2023-03-17 12:37 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-17 10:08 Code quality of some -ts-mode major modes Philip Kaludercic
2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions.
2023-03-17 11:52 ` Eli Zaretskii
2023-03-17 12:37 ` Philip Kaludercic [this message]
2023-03-17 13:54 ` Eli Zaretskii
2023-03-17 15:20 ` Philip Kaludercic
2023-03-17 15:31 ` Eli Zaretskii
2023-03-17 15:49 ` Philip Kaludercic
2023-03-17 16:35 ` Eli Zaretskii
2023-03-17 16:53 ` Philip Kaludercic
2023-03-17 21:45 ` Dmitry Gutov
2023-03-18 5:59 ` Eli Zaretskii
2023-03-17 11:46 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bkkrft9n.fsf@posteo.net \
--to=philipk@posteo.net \
--cc=casouri@gmail.com \
--cc=emacs-devel@gnu.org \
--cc=ruijie@netyu.xyz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).