From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: Code quality of some -ts-mode major modes Date: Fri, 17 Mar 2023 12:37:40 +0000 Message-ID: <87bkkrft9n.fsf@posteo.net> References: <87fsa3g05n.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="25978"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Yuan Fu , emacs-devel@gnu.org To: Ruijie Yu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Mar 17 13:38:13 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pd9LY-0006XC-Kc for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Mar 2023 13:38:12 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pd9Kp-0004Le-OE; Fri, 17 Mar 2023 08:37:27 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pd9Ko-0004LM-Dm for emacs-devel@gnu.org; Fri, 17 Mar 2023 08:37:26 -0400 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pd9Kl-0007zx-Fd for emacs-devel@gnu.org; Fri, 17 Mar 2023 08:37:26 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 4360E2404A3 for ; Fri, 17 Mar 2023 13:37:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1679056641; bh=CwCmzvAxDvrQd5FTvzVxftIV7HUJXmpLh9WMkTIXrVI=; h=From:To:Cc:Subject:Date:From; b=DiYa16B7ICZtqkbDsy9KFhZFR5iwdIy8dK/loIfNUULecAZCBGGIp3gJAbaeMKvhA yoLZwO+GZi/ql8VPY/EysN9azyUILItjU0gYDniyInO5v3VXjtjEWwiNr0jBVOvc6R 0OxZ2LIfb4pV+zgKGJ9c+EqA8dWZSNtPOeFZl5pu6M4X+Bgu5jwzS1lJV/K3PZsgm7 GM1odfqt76r3G3DkiP9wmSUrcvfrN6bjMTYlYDeK9cnTFja5dGt/Bz2KXM2mSxm+1h CO4PdWpRfSbuisglb02Uki0RjsvDSorKFZkCSoBF+UaZodHFpyyLxIXuDP8gdV+NZ0 +F/fjNln1SY5Q== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PdNtl5ShBz6tmF; Fri, 17 Mar 2023 13:37:19 +0100 (CET) In-Reply-To: (Ruijie Yu's message of "Fri, 17 Mar 2023 18:29:52 +0800") Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:304547 Archived-At: Ruijie Yu writes: > Hello Philip, > > Not a maintainer, but wanted to chime in to share some of my > observations and comments. > >> -;; Author : Randy Taylor >> -;; Maintainer : Randy Taylor >> -;; Created : December 2022 >> -;; Keywords : yaml languages tree-sitter >> +;; Author: Randy Taylor >> +;; Maintainer: Randy Taylor >> +;; 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 =E2=80=98Keywords=E2=80=99 and =E2=80=98URL=E2=80=99 headers= are optional, but recommended. The command =E2=80=98describe-package=E2=80=99 uses these to add links = to its output. The =E2=80=98Keywords=E2=80=99 header should contain at least one stand= ard keyword from the =E2=80=98finder-known-keywords=E2=80=99 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 writes: >> > - (when (treesit-ready-p 'yaml) >> > + (when (treesit-ready-p 'yaml) ;why not raise an `user-error= '? >> > (treesit-parser-create 'yaml) >>=20 >> 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?=20=20 Eli Zaretskii writes: >> From: Philip Kaludercic >> Cc: Yuan Fu >> Date: Fri, 17 Mar 2023 10:08:52 +0000 >>=20 >> 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 . >>=20=20 >> ;;; 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: >>=20=20 >> (require 'treesit) >>=20=20 >> -(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.