GNU bug report logs - #61996
30.0.50; Submitting elixir-ts-mode and heex-ts-mode

Previous Next

Full log

Message #32 received at 61996 <at> debbugs.gnu.org (full text, mbox):

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61996 <at> debbugs.gnu.org, theo <at> thornhill.no, casouri <at> gmail.com
Subject: Re: bug#61996: 30.0.50; Submitting elixir-ts-mode and heex-ts-mode
Date: Sat, 11 Mar 2023 20:01:32 +0200

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> >From 88c941067da0e34e1e9ababeb813ba51378ae2cc Mon Sep 17 
>> >00:00:00 2001
>> From: Wilhelm H Kirschbaum <wkirschbaum <at> gmail.com>
>> Date: Mon, 6 Mar 2023 21:18:04 +0200
>> Subject: [PATCH 1/2] Add heex-ts-mode
>> 
>> ---
>>  lisp/progmodes/heex-ts-mode.el                | 185 
>>  ++++++++++++++++++
>>  .../heex-ts-mode-resources/indent.erts        |  47 +++++
>>  test/lisp/progmodes/heex-ts-mode-tests.el     |   9 +
>>  3 files changed, 241 insertions(+)
>>  create mode 100644 lisp/progmodes/heex-ts-mode.el
>>  create mode 100644 
>>  test/lisp/progmodes/heex-ts-mode-resources/indent.erts
>>  create mode 100644 test/lisp/progmodes/heex-ts-mode-tests.el
>
> Please accompany the changes with a commit log message according 
> to
> our conventions (see CONTRIBUTE for the conventions; search for
> "ChangeLog" there).  In this case, just "New file" log should be
> sufficient for the new files you add.
>
Thanks, was not aware of it. I hope it is correct in the new 
patches. 

>> +(declare-function treesit-parser-create "treesit.c")
>> +(declare-function treesit-node-child "treesit.c")
>> +(declare-function treesit-node-type "treesit.c")
>> +(declare-function treesit-install-language-grammar 
>> "treesit.el")
>
> AFAICS, the code uses more functions from treesit.c; please add
> declare-function forms for all of them , to avoid compilation 
> warnings
> n systems where Emacs was built without tree-sitter.
>
I made some changes and checked on a non-treesit build and
see no more warnings. 

>> +(defun heex-ts-mode--forward-sexp (&optional arg)
>> +  (interactive "^p")
>
> Why is a command an internal function?  That is unusual, as 
> commands
> are by definition public.  It looks like you thought the 
> double-hyphen
> "--" notation is a simple delimiter between the package-name 
> part of
> the symbol name and the rest?  If so, you were mistaken: the
> double-hyphen means this is an internal function/variable. 
> Please
> review all your symbol names in this patch and rename as 
> appropriate.
>
> Btw, there's no need to have the prefix be the full name of the
> package, as in "elixir-ts-mode-".  You could use "elixir-ts-" 
> instead.
>
This should be internal, I removed the interactive. 

>> +;;;###autoload
>> +(define-derived-mode heex-ts-mode html-mode "Heex"
>
> html-mode? not html-ts-mode?
>

I don't see the advantage to use html-ts-mode over html-mode at 
the
moment, but can have another look if there is a specific reason to 
do so.

>> >From d13c34ed951e3e6fa473cd1bc2e955e20455022b Mon Sep 17 
>> >00:00:00 2001
>> From: Wilhelm H Kirschbaum <wkirschbaum <at> gmail.com>
>> Date: Mon, 6 Mar 2023 21:18:35 +0200
>> 
>> ---
>>  lisp/progmodes/elixir-ts-mode.el              | 626 
>>  ++++++++++++++++++
>>  .../elixir-ts-mode-resources/indent.erts      | 147 ++++
>>  test/lisp/progmodes/elixir-ts-mode-tests.el   |  31 +
>>  3 files changed, 804 insertions(+)
>>  create mode 100644 lisp/progmodes/elixir-ts-mode.el
>>  create mode 100644 
>>  test/lisp/progmodes/elixir-ts-mode-resources/indent.erts
>>  create mode 100644 test/lisp/progmodes/elixir-ts-mode-tests.el
>
> Likewise here: please add a commit log message describing the 
> changes.
>

>> +(declare-function treesit-parser-create "treesit.c")
>> +(declare-function treesit-node-child "treesit.c")
>> +(declare-function treesit-node-type "treesit.c")
>> +(declare-function treesit-node-child-by-field-name 
>> "treesit.c")
>> +(declare-function treesit-parser-language "treesit.c")
>> +(declare-function treesit-parser-included-ranges "treesit.c")
>> +(declare-function treesit-parser-list "treesit.c")
>> +(declare-function treesit-node-parent "treesit.c")
>> +(declare-function treesit-node-start "treesit.c")
>> +(declare-function treesit-query-compile "treesit.c")
>> +(declare-function treesit-install-language-grammar 
>> "treesit.el")
>
> Please verify that you have declare-function for all the 
> functions
> from treesit.c this package uses, and only for those.
>
I think this is fixed.

>> +(defgroup elixir-ts nil
>> +  "Major mode for editing Ruby code."
>                              ^^^^
> "Ruby"?
>

Copy paste error from ruby-ts-mode when trying to follow 
conventions. 

>> +;; used to distinguish from comment-face in query match
>
> Comments should be complete sentences: start with a capital 
> letter and
> end with a period (here and elsewhere in the patches).
>
>> +(defface elixir-ts-font-comment-doc-identifier-face
>> +  '((t (:inherit font-lock-doc-face)))
>> +  "For use with @comment.doc tag.")
>
> This doc string is too terse.  Imagine someone looking at it in 
> a long
> list of symbols, not necessarily all of them faces.  So 
> something
> like this is better:
>
>   Face used for @comment.doc tags in Elixir files.
>
> Likewise for other faces in the patch.
>
>> +    (modify-syntax-entry ?@ "'" table)
>> +    table)
>> +  "Syntax table for `elixir-ts-mode.")
>                                       ^
> The closing ' quote is missing there.

The new patches should hopefully cover all of the above issues. 
Thanks
for the patience.

[0001-Add-heex-ts-mode-Bug-61996.patch (text/x-patch, attachment)]

[0002-Add-elixir-ts-mode-Bug-61996.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.