* Code quality of some -ts-mode major modes @ 2023-03-17 10:08 Philip Kaludercic 2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions. 2023-03-17 11:46 ` Eli Zaretskii 0 siblings, 2 replies; 13+ messages in thread From: Philip Kaludercic @ 2023-03-17 10:08 UTC (permalink / raw) To: emacs-devel; +Cc: Yuan Fu [-- Attachment #1: Type: text/plain, Size: 494 bytes --] 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: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-diff, Size: 2642 bytes --] 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 [-- Attachment #3: Type: text/plain, Size: 764 bytes --] 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 10:08 Code quality of some -ts-mode major modes Philip Kaludercic @ 2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions. 2023-03-17 11:52 ` Eli Zaretskii 2023-03-17 12:37 ` Philip Kaludercic 2023-03-17 11:46 ` Eli Zaretskii 1 sibling, 2 replies; 13+ messages in thread From: Ruijie Yu via Emacs development discussions. @ 2023-03-17 10:29 UTC (permalink / raw) To: Philip Kaludercic; +Cc: Yuan Fu, emacs-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions. @ 2023-03-17 11:52 ` Eli Zaretskii 2023-03-17 12:37 ` Philip Kaludercic 1 sibling, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2023-03-17 11:52 UTC (permalink / raw) To: Ruijie Yu; +Cc: philipk, casouri, emacs-devel > Cc: Yuan Fu <casouri@gmail.com>, emacs-devel@gnu.org > Date: Fri, 17 Mar 2023 18:29:52 +0800 > From: Ruijie Yu via "Emacs development discussions." <emacs-devel@gnu.org> > > > (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? No, it doesn't. The declare-function are about functions implemented in treesit.c, not treesit.el, so loading the latter cannot possibly fix the need for declare-function in these cases. > Or were these `declare-functions' calls simply there for redundancy? No, they are there to avoid byte-compiler warnings when building Emacs without tree-sitter support (which is optional). > > - (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. We _want_ this to signal an error so that the user is acutely aware his/her system is not configured for these modes. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions. 2023-03-17 11:52 ` Eli Zaretskii @ 2023-03-17 12:37 ` Philip Kaludercic 2023-03-17 13:54 ` Eli Zaretskii 1 sibling, 1 reply; 13+ messages in thread From: Philip Kaludercic @ 2023-03-17 12:37 UTC (permalink / raw) To: Ruijie Yu; +Cc: Yuan Fu, emacs-devel Ruijie Yu <ruijie@netyu.xyz> writes: > 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). 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 ‘Keywords’ and ‘URL’ headers are optional, but recommended. The command ‘describe-package’ uses these to add links to its output. The ‘Keywords’ header should contain at least one standard keyword from the ‘finder-known-keywords’ 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 <eliz@gnu.org> writes: >> > - (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. > > 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? Eli Zaretskii <eliz@gnu.org> writes: >> From: Philip Kaludercic <philipk@posteo.net> >> Cc: Yuan Fu <casouri@gmail.com> >> 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. 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 <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. > > 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: >> >> (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? 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 12:37 ` Philip Kaludercic @ 2023-03-17 13:54 ` Eli Zaretskii 2023-03-17 15:20 ` Philip Kaludercic 2023-03-17 21:45 ` Dmitry Gutov 0 siblings, 2 replies; 13+ messages in thread From: Eli Zaretskii @ 2023-03-17 13:54 UTC (permalink / raw) To: Philip Kaludercic; +Cc: ruijie, casouri, emacs-devel > From: Philip Kaludercic <philipk@posteo.net> > Cc: Yuan Fu <casouri@gmail.com>, emacs-devel@gnu.org > Date: Fri, 17 Mar 2023 12:37:40 +0000 > > >> -(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. That is your personal preference. Objectively, there's nothing wrong with using 'if' that has no 'else' part. So changing someone's code to use 'when' where 'if' can do, or vice versa -- replacing 'when' with a single sexp in the body with 'if' -- has no real justification. > > 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'? Because if we add that to the code, we will need to maintain that for the observable future to be correct. Comments, even if they are outdated, don't need such level of maintenance. Moreover, the fact that a given grammar was used for testing doesn't mean another grammar will not work as well. > >> > - (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. > > > > 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? What is confusing? Again, I explained the rationale many times here. I can explain again, but is that really necessary? > >> 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. Unfortunately, they sound exactly that. Please keep in mind that most of the code of these modes was written by relative newcomers to Emacs development, who had to learn our coding conventions and subtle aspects of Emacs in a very short time, while producing code that is supposed to be stable and solid enough to go into the upcoming release. The challenge which they faced was so tough that frankly I didn't believe they will be able to make it happen. To my surprise and admiration, they did, and did it with flying colors. The issues you mention are real, but they are minor. We can and should fix them without trying to be too judgmental to those who labored on the code under such tense requirements. > >> @@ -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? If it is likely we will want to add options in the near future, then yes. (I just asked a question, I don't have a firm opinion on this matter, and will not object deleting the :group part if we decide a new group is not justified.) > >> - (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. It will also work if the grammar library is installed and the package is loaded, whether manually or not. So I still don't think I understand what confused you. > >> 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? At the beginning of NEWS, where we say that Emacs can be built with the tree-sitter library. > 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]. The assumption is that people either read NEWS in its entirety, or at least search it for "tree-sitter". If they only read parts, then I'm sure they will sometimes be confused. > 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? No, I don't want to do that, see above for the reasons. (We had this discussion about 2 months ago, when the command was added to Emacs.) > >> 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. Help in reviewing patches when they are posted is also very welcome. It takes more than one pair of eyes to spot every bit that needs attention. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 13:54 ` Eli Zaretskii @ 2023-03-17 15:20 ` Philip Kaludercic 2023-03-17 15:31 ` Eli Zaretskii 2023-03-17 21:45 ` Dmitry Gutov 1 sibling, 1 reply; 13+ messages in thread From: Philip Kaludercic @ 2023-03-17 15:20 UTC (permalink / raw) To: Eli Zaretskii; +Cc: ruijie, casouri, emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> From: Philip Kaludercic <philipk@posteo.net> >> Cc: Yuan Fu <casouri@gmail.com>, emacs-devel@gnu.org >> Date: Fri, 17 Mar 2023 12:37:40 +0000 >> >> >> -(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. > > That is your personal preference. Objectively, there's nothing wrong > with using 'if' that has no 'else' part. So changing someone's code > to use 'when' where 'if' can do, or vice versa -- replacing 'when' > with a single sexp in the body with 'if' -- has no real justification. Technically no, but I do hope not to be mistaken that there is a convention (along the lines I gave above) here that goes beyond just my personal preference. CLTL even says[p. 166]: As a matter of style, when is normally used to conditionally produce some side effects, and the value of the when form is normally not used. If the value is relevant, then it may be stylistically more appropriate to use and or if. >> > 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'? > > Because if we add that to the code, we will need to maintain that for > the observable future to be correct. Comments, even if they are > outdated, don't need such level of maintenance. That could be resolved by either pinning a revision or instead of cloning the repository to download a tarball of a tag. In fact that should make the system even more stable than the way I see it being promoted around the web currently, that just maps languages to Git repository URLs. > Moreover, the fact > that a given grammar was used for testing doesn't mean another grammar > will not work as well. I don't know of any language with multiple independent implementations (that is out of ignorance, not omniscience). For that reason I don't know how they would behave or how robust the Emacs implementations are when the behaviour changes? >> >> > - (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. >> > >> > 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? > > What is confusing? > > Again, I explained the rationale many times here. I can explain > again, but is that really necessary? You had previously said that you are opposed to raising an error (or am I mistaken?), while the above comment says "we want this to signal and error". >> >> 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. > > Unfortunately, they sound exactly that. I know, but as this was not intended I wanted to clarify that this was not the case. Either way it is not important -- as most of these issues are non-technical it is just difficult to object with reference to some objective point. > Please keep in mind that most of the code of these modes was written > by relative newcomers to Emacs development, who had to learn our > coding conventions and subtle aspects of Emacs in a very short time, > while producing code that is supposed to be stable and solid enough to > go into the upcoming release. The challenge which they faced was so > tough that frankly I didn't believe they will be able to make it > happen. To my surprise and admiration, they did, and did it with > flying colors. The issues you mention are real, but they are minor. > We can and should fix them without trying to be too judgmental to > those who labored on the code under such tense requirements. Sure, I just noticed how these "issues" (if we even want to call them that) were spreading, which I argue is the greater "issue". >> >> @@ -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? > > If it is likely we will want to add options in the near future, then > yes. (I just asked a question, I don't have a firm opinion on this > matter, and will not object deleting the :group part if we decide a > new group is not justified.) Neither to I have a firm opinion, except that a broken reference should be avoided. >> >> - (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. > > It will also work if the grammar library is installed and the package > is loaded, whether manually or not. So I still don't think I > understand what confused you. I must have misunderstood something completely, because I can't even parse your response. It is probably best to set this aside for now. I'll bring it up if I at some later point have a better grasp on the details and still believe this to be an issue. >> >> 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? > > At the beginning of NEWS, where we say that Emacs can be built with > the tree-sitter library. OK, missed that. >> 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]. > > The assumption is that people either read NEWS in its entirety, or at > least search it for "tree-sitter". If they only read parts, then I'm > sure they will sometimes be confused. Fair point. >> 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? > > No, I don't want to do that, see above for the reasons. (We had this > discussion about 2 months ago, when the command was added to Emacs.) I know but I didn't think a satisfying conclusion was found, and I couldn't continue the discussion back then due to time constraints. >> >> 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. > > Help in reviewing patches when they are posted is also very welcome. > It takes more than one pair of eyes to spot every bit that needs > attention. I'll try and do so when I notice one. I have also been sketching out support for a markdown-ts-mode to better understand the how tree-sitter works, which could help. -- Philip Kaludercic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 15:20 ` Philip Kaludercic @ 2023-03-17 15:31 ` Eli Zaretskii 2023-03-17 15:49 ` Philip Kaludercic 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2023-03-17 15:31 UTC (permalink / raw) To: Philip Kaludercic; +Cc: ruijie, casouri, emacs-devel > From: Philip Kaludercic <philipk@posteo.net> > Cc: ruijie@netyu.xyz, casouri@gmail.com, emacs-devel@gnu.org > Date: Fri, 17 Mar 2023 15:20:49 +0000 > > >> 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. > > > > That is your personal preference. Objectively, there's nothing wrong > > with using 'if' that has no 'else' part. So changing someone's code > > to use 'when' where 'if' can do, or vice versa -- replacing 'when' > > with a single sexp in the body with 'if' -- has no real justification. > > Technically no, but I do hope not to be mistaken that there is a > convention (along the lines I gave above) here that goes beyond just my > personal preference. CLTL even says[p. 166]: It is fine for you to prefer this convention, but we don't mandate it in Emacs. > > Because if we add that to the code, we will need to maintain that for > > the observable future to be correct. Comments, even if they are > > outdated, don't need such level of maintenance. > > That could be resolved by either pinning a revision or instead of > cloning the repository to download a tarball of a tag. In fact that > should make the system even more stable than the way I see it being > promoted around the web currently, that just maps languages to Git > repository URLs. It can be resolved in more than one way, but all of them mean additional maintenance burden, so I don't think we should undertake that. > > Moreover, the fact > > that a given grammar was used for testing doesn't mean another grammar > > will not work as well. > > I don't know of any language with multiple independent implementations I do. Indeed, most have just one. But not all. > > Again, I explained the rationale many times here. I can explain > > again, but is that really necessary? > > You had previously said that you are opposed to raising an error (or am > I mistaken?), while the above comment says "we want this to signal and > error". No you are mistaken. We do want this to signal an error if tree-sitter is not compiled in or the grammar is not available. > > Help in reviewing patches when they are posted is also very welcome. > > It takes more than one pair of eyes to spot every bit that needs > > attention. > > I'll try and do so when I notice one. I have also been sketching out > support for a markdown-ts-mode to better understand the how tree-sitter > works, which could help. TIA. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 15:31 ` Eli Zaretskii @ 2023-03-17 15:49 ` Philip Kaludercic 2023-03-17 16:35 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Philip Kaludercic @ 2023-03-17 15:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: ruijie, casouri, emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> From: Philip Kaludercic <philipk@posteo.net> >> Cc: ruijie@netyu.xyz, casouri@gmail.com, emacs-devel@gnu.org >> Date: Fri, 17 Mar 2023 15:20:49 +0000 >> >> >> 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. >> > >> > That is your personal preference. Objectively, there's nothing wrong >> > with using 'if' that has no 'else' part. So changing someone's code >> > to use 'when' where 'if' can do, or vice versa -- replacing 'when' >> > with a single sexp in the body with 'if' -- has no real justification. >> >> Technically no, but I do hope not to be mistaken that there is a >> convention (along the lines I gave above) here that goes beyond just my >> personal preference. CLTL even says[p. 166]: > > It is fine for you to prefer this convention, but we don't mandate it > in Emacs. Ok, in that case I was mistaken. I have just seen a number of commits that either just or also made this change, making it seem like established knowledge. >> > Because if we add that to the code, we will need to maintain that for >> > the observable future to be correct. Comments, even if they are >> > outdated, don't need such level of maintenance. >> >> That could be resolved by either pinning a revision or instead of >> cloning the repository to download a tarball of a tag. In fact that >> should make the system even more stable than the way I see it being >> promoted around the web currently, that just maps languages to Git >> repository URLs. > > It can be resolved in more than one way, but all of them mean > additional maintenance burden, so I don't think we should undertake > that. If you think so, but at least we agree that one reference to a grammar would be helpful. >> > Moreover, the fact >> > that a given grammar was used for testing doesn't mean another grammar >> > will not work as well. >> >> I don't know of any language with multiple independent implementations > > I do. Indeed, most have just one. But not all. Could you give me an example that I could try out? >> > Again, I explained the rationale many times here. I can explain >> > again, but is that really necessary? >> >> You had previously said that you are opposed to raising an error (or am >> I mistaken?), while the above comment says "we want this to signal and >> error". > > No you are mistaken. We do want this to signal an error if > tree-sitter is not compiled in or the grammar is not available. But it doesn't, or am I completely oblivious that (treesit-ready-p 'something-i-just-made-up) just displays a warning? The way I am currently reading/seeing the code is that if tree-sitter and the grammar are available, then it will be initialised but otherwise we just get a warning and a pretty plain mode. >> > Help in reviewing patches when they are posted is also very welcome. >> > It takes more than one pair of eyes to spot every bit that needs >> > attention. >> >> I'll try and do so when I notice one. I have also been sketching out >> support for a markdown-ts-mode to better understand the how tree-sitter >> works, which could help. > > TIA. -- Philip Kaludercic ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 15:49 ` Philip Kaludercic @ 2023-03-17 16:35 ` Eli Zaretskii 2023-03-17 16:53 ` Philip Kaludercic 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2023-03-17 16:35 UTC (permalink / raw) To: Philip Kaludercic; +Cc: ruijie, casouri, emacs-devel > From: Philip Kaludercic <philipk@posteo.net> > Cc: ruijie@netyu.xyz, casouri@gmail.com, emacs-devel@gnu.org > Date: Fri, 17 Mar 2023 15:49:15 +0000 > > Eli Zaretskii <eliz@gnu.org> writes: > > >> I don't know of any language with multiple independent implementations > > > > I do. Indeed, most have just one. But not all. > > Could you give me an example that I could try out? One example I know of is sql. I think there's at least one other, but I don't remember which one. > > No you are mistaken. We do want this to signal an error if > > tree-sitter is not compiled in or the grammar is not available. > > But it doesn't, or am I completely oblivious that > > (treesit-ready-p 'something-i-just-made-up) > > just displays a warning? I'm not sure I understand the question. Are you saying that the above is silent, or are you saying that it pops up a warning instead of signaling an error? If the latter, this is the intended behavior; sorry for confusingly saying it should error out. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 16:35 ` Eli Zaretskii @ 2023-03-17 16:53 ` Philip Kaludercic 0 siblings, 0 replies; 13+ messages in thread From: Philip Kaludercic @ 2023-03-17 16:53 UTC (permalink / raw) To: Eli Zaretskii; +Cc: ruijie, casouri, emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> From: Philip Kaludercic <philipk@posteo.net> >> Cc: ruijie@netyu.xyz, casouri@gmail.com, emacs-devel@gnu.org >> Date: Fri, 17 Mar 2023 15:49:15 +0000 >> >> Eli Zaretskii <eliz@gnu.org> writes: >> >> >> I don't know of any language with multiple independent implementations >> > >> > I do. Indeed, most have just one. But not all. >> >> Could you give me an example that I could try out? > > One example I know of is sql. I think there's at least one other, but > I don't remember which one. For posterity, I'll just mentioned that I managed to find two: - https://github.com/DerekStride/tree-sitter-sql ("general/permissive") - https://github.com/m-novikov/tree-sitter-sql ("focus on postgresql") >> > No you are mistaken. We do want this to signal an error if >> > tree-sitter is not compiled in or the grammar is not available. >> >> But it doesn't, or am I completely oblivious that >> >> (treesit-ready-p 'something-i-just-made-up) >> >> just displays a warning? > > I'm not sure I understand the question. Are you saying that the above > is silent, or are you saying that it pops up a warning instead of > signaling an error? If the latter, this is the intended behavior; > sorry for confusingly saying it should error out. The latter -- that also explains my confusion. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 13:54 ` Eli Zaretskii 2023-03-17 15:20 ` Philip Kaludercic @ 2023-03-17 21:45 ` Dmitry Gutov 2023-03-18 5:59 ` Eli Zaretskii 1 sibling, 1 reply; 13+ messages in thread From: Dmitry Gutov @ 2023-03-17 21:45 UTC (permalink / raw) To: Eli Zaretskii, Philip Kaludercic; +Cc: ruijie, casouri, emacs-devel On 17/03/2023 15:54, Eli Zaretskii wrote: >> 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? > No, I don't want to do that, see above for the reasons. (We had this > discussion about 2 months ago, when the command was added to Emacs.) FWIW, I had no problems with that conclusion, but then I noticed that we keep a separate list of sources inside admin/notes/tree-sitter, which basically contains the same info, as well as all 6 known exceptions to the general scenario (where in the usual case we can determine everything just from the name of the language). If we're going to keep it updated (and we apparently are), why not move that info to treesit-language-source-alist instead. We can make it a defcustom, to emphasize that people should update it whenever they see the data is old. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 21:45 ` Dmitry Gutov @ 2023-03-18 5:59 ` Eli Zaretskii 0 siblings, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2023-03-18 5:59 UTC (permalink / raw) To: Dmitry Gutov; +Cc: philipk, ruijie, casouri, emacs-devel > Date: Fri, 17 Mar 2023 23:45:00 +0200 > Cc: ruijie@netyu.xyz, casouri@gmail.com, emacs-devel@gnu.org > From: Dmitry Gutov <dgutov@yandex.ru> > > On 17/03/2023 15:54, Eli Zaretskii wrote: > >> 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? > > No, I don't want to do that, see above for the reasons. (We had this > > discussion about 2 months ago, when the command was added to Emacs.) > > FWIW, I had no problems with that conclusion, but then I noticed that we > keep a separate list of sources inside admin/notes/tree-sitter, which > basically contains the same info, as well as all 6 known exceptions to > the general scenario (where in the usual case we can determine > everything just from the name of the language). > > If we're going to keep it updated (and we apparently are), why not move > that info to treesit-language-source-alist instead. My original plan was to remove that script from admin/notes before releasing Emacs 29. I decided to wait for a while, but I definitely don't want to commit ourselves to maintaining the script in the future versions. The information is just one Internet search away, usually the first hit is the one you want. More generally, the tree-sitter stuff seems to be just starting to get noticed by distros, and it is too early to make any firm decisions related to availability of information and libraries. We already did in this case more than we usually do for optional features, and I think it should be enough for now. Further efforts that increase our maintenance burden should wait until it is clear they are necessary. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Code quality of some -ts-mode major modes 2023-03-17 10:08 Code quality of some -ts-mode major modes Philip Kaludercic 2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions. @ 2023-03-17 11:46 ` Eli Zaretskii 1 sibling, 0 replies; 13+ messages in thread From: Eli Zaretskii @ 2023-03-17 11:46 UTC (permalink / raw) To: Philip Kaludercic; +Cc: emacs-devel, casouri > From: Philip Kaludercic <philipk@posteo.net> > Cc: Yuan Fu <casouri@gmail.com> > 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 <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. 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-03-18 5:59 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-17 10:08 Code quality of some -ts-mode major modes Philip Kaludercic 2023-03-17 10:29 ` Ruijie Yu via Emacs development discussions. 2023-03-17 11:52 ` Eli Zaretskii 2023-03-17 12:37 ` Philip Kaludercic 2023-03-17 13:54 ` Eli Zaretskii 2023-03-17 15:20 ` Philip Kaludercic 2023-03-17 15:31 ` Eli Zaretskii 2023-03-17 15:49 ` Philip Kaludercic 2023-03-17 16:35 ` Eli Zaretskii 2023-03-17 16:53 ` Philip Kaludercic 2023-03-17 21:45 ` Dmitry Gutov 2023-03-18 5:59 ` Eli Zaretskii 2023-03-17 11:46 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).