From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Code quality of some -ts-mode major modes Date: Fri, 17 Mar 2023 13:46:24 +0200 Message-ID: <83lejveh2n.fsf@gnu.org> References: <87fsa3g05n.fsf@posteo.net> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4164"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org, casouri@gmail.com To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Mar 17 12:47:09 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 1pd8Y9-0000qV-0P for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Mar 2023 12:47:09 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pd8XU-000518-E5; Fri, 17 Mar 2023 07:46:28 -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 1pd8XT-0004zB-16 for emacs-devel@gnu.org; Fri, 17 Mar 2023 07:46:27 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pd8XR-0006uD-Ng; Fri, 17 Mar 2023 07:46:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=USUjVEfp71UXPExHFFEFujGO67PFGUbSv2uhWATMwN0=; b=ngwOfHXiThEu NvqCpBI+80OhIvYW4BFA8yZzFSrQchBP14nKD1axCgQiRurQCYnweJ5bCdlSMBNB3EfNcm4Mki2Ji JTlo4pgaQNPt4e6zR82g04DFRQhTa6OZst4xeaOSP+BaTuyTDm3dJSpS7Wl436S3FehqmY/R/rstT K9FE9EXvDvr8nnrPDnZZJF27vRDGi3O/QorPau9/haUis+TfIjz/gnmpCdmpcz4kd3orJ8o7VnWfn gw9AEXnxlZN+H0pV3S8SB5EOKVWxn7i3AIWrhVBUgfMAUzRyD6NDyOV08HT/jtiXOk7OG0lc6udkv JnafXX27UVhrLZAXvqIPyQ==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pd8XQ-0006Od-OC; Fri, 17 Mar 2023 07:46:25 -0400 In-Reply-To: <87fsa3g05n.fsf@posteo.net> (message from Philip Kaludercic on Fri, 17 Mar 2023 10:08:52 +0000) 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:304544 Archived-At: > From: Philip Kaludercic > Cc: Yuan Fu > 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. > @@ -23,15 +23,18 @@ > ;; along with GNU Emacs. If not, see . > > ;;; 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. > ;;; 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? > - (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. > 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. > 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. > 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.