From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Ruijie Yu via "Emacs development discussions." Newsgroups: gmane.emacs.devel Subject: Re: Code quality of some -ts-mode major modes Date: Fri, 17 Mar 2023 18:29:52 +0800 Message-ID: References: <87fsa3g05n.fsf@posteo.net> Reply-To: Ruijie Yu Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="15014"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: mu4e 1.8.14; emacs 30.0.50 Cc: Yuan Fu , emacs-devel@gnu.org To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Mar 17 12:08:47 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 1pd7wz-0003gX-Li for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Mar 2023 12:08:45 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pd7wI-0003J9-7p; Fri, 17 Mar 2023 07:08:02 -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 1pd7wG-0003Iw-M7 for emacs-devel@gnu.org; Fri, 17 Mar 2023 07:08:00 -0400 Original-Received: from netyu.xyz ([152.44.41.246] helo=mail.netyu.xyz) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pd7wD-0001tj-CJ for emacs-devel@gnu.org; Fri, 17 Mar 2023 07:08:00 -0400 Original-Received: from fw.net.yu.netyu.xyz ( [222.248.4.98]) by netyu.xyz (OpenSMTPD) with ESMTPSA id 36358452 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Fri, 17 Mar 2023 11:07:50 +0000 (UTC) In-reply-to: <87fsa3g05n.fsf@posteo.net> Received-SPF: pass client-ip=152.44.41.246; envelope-from=ruijie@netyu.xyz; helo=mail.netyu.xyz X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, 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:304542 Archived-At: 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). I also realize you have removed the keywords "yaml" and "tree-sitter", why so? > -;; > + > +;; 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? > -(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. > 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. > [...] -- Best, RY