[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Code quality of some -ts-mode major modes
From: |
Ruijie Yu |
Subject: |
Re: Code quality of some -ts-mode major modes |
Date: |
Fri, 17 Mar 2023 18:29:52 +0800 |
User-agent: |
mu4e 1.8.14; emacs 30.0.50 |
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).
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
- Code quality of some -ts-mode major modes, Philip Kaludercic, 2023/03/17
- Re: Code quality of some -ts-mode major modes,
Ruijie Yu <=
- Re: Code quality of some -ts-mode major modes, Eli Zaretskii, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Philip Kaludercic, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Eli Zaretskii, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Philip Kaludercic, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Eli Zaretskii, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Philip Kaludercic, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Eli Zaretskii, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Philip Kaludercic, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Dmitry Gutov, 2023/03/17
- Re: Code quality of some -ts-mode major modes, Eli Zaretskii, 2023/03/18