emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Code quality of some -ts-mode major modes


From: Philip Kaludercic
Subject: Code quality of some -ts-mode major modes
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.

Take `yaml-ts-mode' for example.  I attach a diff with a few changes
that I would have expected to have been made before the file was merged:

diff --git a/lisp/textmodes/yaml-ts-mode.el b/lisp/textmodes/yaml-ts-mode.el
index dfa8d22fb34..43e8868fc44 100644
--- a/lisp/textmodes/yaml-ts-mode.el
+++ b/lisp/textmodes/yaml-ts-mode.el
@@ -2,10 +2,10 @@
 
 ;; Copyright (C) 2022-2023 Free Software Foundation, Inc.
 
-;; 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 file is part of GNU Emacs.
 
@@ -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.
 
 ;;; Code:
 
 (require 'treesit)
 
-(declare-function treesit-parser-create "treesit.c")
+;; (declare-function treesit-parser-create "treesit.c") ;doesn't appear 
necessary
 
-(defvar yaml-ts-mode--syntax-table
+(defvar yaml-ts-mode-syntax-table
   (let ((table (make-syntax-table)))
     (modify-syntax-entry ?#  "<"  table)
     (modify-syntax-entry ?\n ">"  table)
@@ -59,7 +62,8 @@ yaml-ts-mode--font-lock-settings
       (null_scalar)
       (reserved_directive)
       (tag_directive)
-      (yaml_directive)] @font-lock-constant-face)
+      (yaml_directive)]
+     @font-lock-constant-face)
 
    :language 'yaml
    :feature 'delimiter
@@ -83,7 +87,8 @@ yaml-ts-mode--font-lock-settings
    '([(block_scalar)
       (double_quote_scalar)
       (single_quote_scalar)
-      (string_scalar)] @font-lock-string-face)
+      (string_scalar)]
+     @font-lock-string-face)
 
    :language 'yaml
    :feature 'escape-sequence
@@ -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?
 
-  (when (treesit-ready-p 'yaml)
+  (when (treesit-ready-p 'yaml)         ;why not raise an `user-error'?
     (treesit-parser-create 'yaml)
 
     ;; Comments.
@@ -143,9 +147,8 @@ yaml-ts-mode
 
     (treesit-major-mode-setup)))
 
-(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)))
 
 (provide 'yaml-ts-mode)
-
 ;;; yaml-ts-mode.el ends here
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.

My question: Would there be any objection from those involves with
tree-sitter against me making changes like the ones I gave above?
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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]