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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61996 in the body.
You can then email your comments to 61996 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: casouri <at> gmail.com, theo <at> thornhill.no
Subject: 30.0.50; Submitting elixir-ts-mode and heex-ts-mode
Date: Mon, 06 Mar 2023 09:04:13 +0200

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

I would like to submit elixir-ts-mode and heex-ts-mode to emacs 
master.

The package elixir-ts-mode and its dependency heex-ts-mode is
currently a melpa package: https://melpa.org/#/elixir-ts-mode. 
This is a
slightly simplified version, also authored by me.

There is one change not authored by me:
https://github.com/wkirschbaum/elixir-ts-mode/commit/21ad74877ebb55f4bf0b31c2f463bbfda72590ef
which is a duplication removal.

I completed the assignment process in Jan.

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

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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: Mon, 06 Mar 2023 13:59:02 +0200

> Cc: casouri <at> gmail.com, theo <at> thornhill.no
> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Date: Mon, 06 Mar 2023 09:04:13 +0200
> 
> I would like to submit elixir-ts-mode and heex-ts-mode to emacs 
> master.

Thanks.  Please state in the comments to each mode with which grammars
is it compatible, so that users could know from which URL to download
the required grammar libraries.  The heex-ts-mode mentions that, but
elixir-ts-mode doesn't, AFAICT.

> +(defcustom heex-ts-mode-indent-offset 2
> +  "Indentation of Heex statements."
> +  :version "29.1"

I think these modes should go to the master branch, so "30.1" is more
accurate.

> +(if (treesit-ready-p 'elixir)
> +    (progn
> +      (add-to-list 'auto-mode-alist '("\\.elixir\\'" . elixir-ts-mode))
> +      (add-to-list 'auto-mode-alist '("\\.ex\\'" . elixir-ts-mode))
> +      (add-to-list 'auto-mode-alist '("\\.exs\\'" . elixir-ts-mode))
> +      (add-to-list 'auto-mode-alist '("mix\\.lock" . elixir-ts-mode))))
> +
> +(if (treesit-ready-p 'heex)
> +    (add-to-list 'auto-mode-alist '("\\.[hl]?eex\\'" . heex-ts-mode)))
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy/paste error, I presume?

Thanks.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>, 61996 <at> debbugs.gnu.org
Cc: casouri <at> gmail.com, theo <at> thornhill.no
Subject: Re: bug#61996: 30.0.50; Submitting elixir-ts-mode and heex-ts-mode
Date: Mon, 6 Mar 2023 18:41:56 +0200

Hi!

On 06/03/2023 09:04, Wilhelm Kirschbaum wrote:
> I would like to submit elixir-ts-mode and heex-ts-mode to emacs master.

It would be great if you could accompany it with some testing suite: 
indentation code is famously prone to regressions.

You can see the examples of such tests for c-ts-mode or ruby-ts-mode 
(they use different approaches).

Message #14 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: Mon, 06 Mar 2023 19:23:39 +0200

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

>> Cc: casouri <at> gmail.com, theo <at> thornhill.no
>> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
>> Date: Mon, 06 Mar 2023 09:04:13 +0200
>> 
>> I would like to submit elixir-ts-mode and heex-ts-mode to emacs 
>> master.
>
> Thanks.  Please state in the comments to each mode with which 
> grammars
> is it compatible, so that users could know from which URL to 
> download
> the required grammar libraries.  The heex-ts-mode mentions that, 
> but
> elixir-ts-mode doesn't, AFAICT.
>

Will this make sense in the Commentary section: "The
tree-sitter grammar for Elixir can be downloaded from 
https://github.com/phoenixframework/tree-sitter-heex."

>> +(defcustom heex-ts-mode-indent-offset 2
>> +  "Indentation of Heex statements."
>> +  :version "29.1"
>
> I think these modes should go to the master branch, so "30.1" is 
> more
> accurate.
>

Thanks, will change.

>> +(if (treesit-ready-p 'elixir)
>> +    (progn
>> +      (add-to-list 'auto-mode-alist '("\\.elixir\\'" . 
>> elixir-ts-mode))
>> +      (add-to-list 'auto-mode-alist '("\\.ex\\'" . 
>> elixir-ts-mode))
>> +      (add-to-list 'auto-mode-alist '("\\.exs\\'" . 
>> elixir-ts-mode))
>> +      (add-to-list 'auto-mode-alist '("mix\\.lock" . 
>> elixir-ts-mode))))
>> +
>> +(if (treesit-ready-p 'heex)
>> +    (add-to-list 'auto-mode-alist '("\\.[hl]?eex\\'" . 
>> heex-ts-mode)))
>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Copy/paste error, I presume?
>
> Thanks.

This was intentional, but perhaps a bad choice and lack of 
understanding of how the
-ts-modes should be activated. elixir-ts-mode should ideally load 
the
HEEx grammar, but should also technically be able to function 
without.
The Elixir language author mentioned that heex can practically be 
seen
as part of Elixir, so requiring heex-ts-mode makes sense, sort of.

heex-ts-mode and elixir-ts-mode used to be in one file, but I was 
asked to
split them for the MELPA submission. The HEEx language should 
actually
also be able to embed Elixir, but this is not essential and we can 
do
without imo. Would it make sense have them in one file? 

I will update the patch with the above changes including some 
tests. 

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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: Mon, 06 Mar 2023 20:36:54 +0200

> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
> Date: Mon, 06 Mar 2023 19:23:39 +0200
> 
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Thanks.  Please state in the comments to each mode with which
> > grammars is it compatible, so that users could know from which URL
> > to download the required grammar libraries.  The heex-ts-mode
> > mentions that, but elixir-ts-mode doesn't, AFAICT.
> 
> Will this make sense in the Commentary section: "The
> tree-sitter grammar for Elixir can be downloaded from 
> https://github.com/phoenixframework/tree-sitter-heex."

Yes, that's good enough.  But please change the wording to say that
this is the grammar with which the package was tested, not just that
it "can be downloaded" from that place.  That way, users will know
that if they use a different grammar for the same language, they might
be on their own.

> >> +(if (treesit-ready-p 'elixir)
> >> +    (progn
> >> +      (add-to-list 'auto-mode-alist '("\\.elixir\\'" . 
> >> elixir-ts-mode))
> >> +      (add-to-list 'auto-mode-alist '("\\.ex\\'" . 
> >> elixir-ts-mode))
> >> +      (add-to-list 'auto-mode-alist '("\\.exs\\'" . 
> >> elixir-ts-mode))
> >> +      (add-to-list 'auto-mode-alist '("mix\\.lock" . 
> >> elixir-ts-mode))))
> >> +
> >> +(if (treesit-ready-p 'heex)
> >> +    (add-to-list 'auto-mode-alist '("\\.[hl]?eex\\'" . 
> >> heex-ts-mode)))
> >                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Copy/paste error, I presume?
> >
> > Thanks.
> 
> This was intentional, but perhaps a bad choice and lack of 
> understanding of how the
> -ts-modes should be activated. elixir-ts-mode should ideally load 
>  the
> HEEx grammar, but should also technically be able to function 
> without.

Sorry, I don't understand: are you saying that the HEEx grammar
supports both modes?  I thought you need a separate grammar for
Elixir.  I also thought the Elixir files have different file-name
extensions than the HEEx files.  Was I mistaken?

> The Elixir language author mentioned that heex can practically be
> seen as part of Elixir, so requiring heex-ts-mode makes sense, sort
> of.
> 
> heex-ts-mode and elixir-ts-mode used to be in one file, but I was
> asked to split them for the MELPA submission. The HEEx language
> should actually also be able to embed Elixir, but this is not
> essential and we can do without imo. Would it make sense have them
> in one file?

Maybe.  Otherwise, if they have a lot in common, you'd need to
duplicate stuff or have a common file used by both.  Your call.

Message #20 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: Mon, 06 Mar 2023 21:24:11 +0200

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

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

>> >> +(if (treesit-ready-p 'elixir)
>> >> +    (progn
>> >> +      (add-to-list 'auto-mode-alist '("\\.elixir\\'" . 
>> >> elixir-ts-mode))
>> >> +      (add-to-list 'auto-mode-alist '("\\.ex\\'" . 
>> >> elixir-ts-mode))
>> >> +      (add-to-list 'auto-mode-alist '("\\.exs\\'" . 
>> >> elixir-ts-mode))
>> >> +      (add-to-list 'auto-mode-alist '("mix\\.lock" . 
>> >> elixir-ts-mode))))
>> >> +
>> >> +(if (treesit-ready-p 'heex)
>> >> +    (add-to-list 'auto-mode-alist '("\\.[hl]?eex\\'" . 
>> >> heex-ts-mode)))
>> >                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > Copy/paste error, I presume?
>> >
>> > Thanks.
>> 
>> This was intentional, but perhaps a bad choice and lack of 
>> understanding of how the
>> -ts-modes should be activated. elixir-ts-mode should ideally 
>> load 
>>  the
>> HEEx grammar, but should also technically be able to function 
>> without.
>
> Sorry, I don't understand: are you saying that the HEEx grammar
> supports both modes?  I thought you need a separate grammar for
> Elixir.  I also thought the Elixir files have different 
> file-name
> extensions than the HEEx files.  Was I mistaken?
>

No, you were not mistaken. I corrected this with the new patches.

>> The Elixir language author mentioned that heex can practically 
>> be
>> seen as part of Elixir, so requiring heex-ts-mode makes sense, 
>> sort
>> of.
>> 
>> heex-ts-mode and elixir-ts-mode used to be in one file, but I 
>> was
>> asked to split them for the MELPA submission. The HEEx language
>> should actually also be able to embed Elixir, but this is not
>> essential and we can do without imo. Would it make sense have 
>> them
>> in one file?
>
> Maybe.  Otherwise, if they have a lot in common, you'd need to
> duplicate stuff or have a common file used by both.  Your call.

For this release it will be good just to get the basics to work as 
is, but
good to know it is an option. 

Attached are the updated patches. Is this the right format, or 
should I
add them inline rather?

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

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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: Mon, 06 Mar 2023 22:14:37 +0200

> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
> Date: Mon, 06 Mar 2023 21:24:11 +0200
> 
> Attached are the updated patches. Is this the right format, or
> should I add them inline rather?

It's okay either way.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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 11:16:08 +0200

> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
> Date: Mon, 06 Mar 2023 21:24:11 +0200
> 
> For this release it will be good just to get the basics to work as
> is, but good to know it is an option.
> 
> Attached are the updated patches.

Thanks, a few minor comments below.

> >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.

> +(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.

> +(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.

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

html-mode? not html-ts-mode?

> >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.

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

> +;; 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.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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 16:16:24 +0200

On 11/03/2023 11:16, Eli Zaretskii wrote:
>> +(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.

I'm guessing it was made interactive for debugging purposes.

But even that doesn't seem necessary: calling 'forward-sexp' through its 
regular binding will invoke forward-sexp-function basically right away.

(treesit-forward-sexp doesn't need to be interactive either.)

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)]

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

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

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> On 11/03/2023 11:16, Eli Zaretskii wrote:
>>> +(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.
>
> I'm guessing it was made interactive for debugging purposes.
>
> But even that doesn't seem necessary: calling 'forward-sexp' 
> through
> its regular binding will invoke forward-sexp-function basically 
> right
> away.
>
> (treesit-forward-sexp doesn't need to be interactive either.)

This was a mistake and still learning some basic conventions. I 
don't see a
reason to call this function interactively.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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: Sun, 12 Mar 2023 11:00:46 +0200

> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
> Date: Sat, 11 Mar 2023 20:01:32 +0200
> 
> > 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. 

Yes, it is.  Thanks.

> The new patches should hopefully cover all of the above issues. 

Almost there.  Byte compiler shows warnings, which I think are real
problems in the code:

  In elixir-ts--call-parent-start:
  progmodes/elixir-ts-mode.el:459:38: Warning: Unused lexical argument `node'
  progmodes/elixir-ts-mode.el:463:15: Warning: reference to free variable `parent'

  In elixir-ts--forward-sexp:
  progmodes/elixir-ts-mode.el:482:8: Warning: reference to free variable `heex-ts--sexp-regexp'

Message #41 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: Sun, 12 Mar 2023 11:54:33 +0200

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

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

>> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
>> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
>> Date: Sat, 11 Mar 2023 20:01:32 +0200
>> 
>> > 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. 
>
> Yes, it is.  Thanks.
>
>> The new patches should hopefully cover all of the above issues. 
>
> Almost there.  Byte compiler shows warnings, which I think are 
> real
> problems in the code:
>
>   In elixir-ts--call-parent-start:
>   progmodes/elixir-ts-mode.el:459:38: Warning: Unused lexical 
>   argument `node'
>   progmodes/elixir-ts-mode.el:463:15: Warning: reference to free 
>   variable `parent'
>
>   In elixir-ts--forward-sexp:
>   progmodes/elixir-ts-mode.el:482:8: Warning: reference to free 
>   variable `heex-ts--sexp-regexp'

Ah, not sure how I missed them. The new patches have further 
tweaks and
should resolve the above issue.

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

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
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: Sun, 12 Mar 2023 13:37:21 +0200

> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
> Date: Sun, 12 Mar 2023 11:54:33 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >   In elixir-ts--call-parent-start:
> >   progmodes/elixir-ts-mode.el:459:38: Warning: Unused lexical 
> >   argument `node'
> >   progmodes/elixir-ts-mode.el:463:15: Warning: reference to free 
> >   variable `parent'
> >
> >   In elixir-ts--forward-sexp:
> >   progmodes/elixir-ts-mode.el:482:8: Warning: reference to free 
> >   variable `heex-ts--sexp-regexp'
> 
> Ah, not sure how I missed them. The new patches have further 
> tweaks and
> should resolve the above issue.

Thanks, but the first of the two patches lacks the commit log
message.  And since you said there are further tweaks, I wasn't sure
the one from the previous version was still accurate.

Message #47 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: Sun, 12 Mar 2023 14:23:01 +0200

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

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

>> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
>> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
>> Date: Sun, 12 Mar 2023 11:54:33 +0200
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >   In elixir-ts--call-parent-start:
>> >   progmodes/elixir-ts-mode.el:459:38: Warning: Unused lexical 
>> >   argument `node'
>> >   progmodes/elixir-ts-mode.el:463:15: Warning: reference to 
>> >   free 
>> >   variable `parent'
>> >
>> >   In elixir-ts--forward-sexp:
>> >   progmodes/elixir-ts-mode.el:482:8: Warning: reference to 
>> >   free 
>> >   variable `heex-ts--sexp-regexp'
>> 
>> Ah, not sure how I missed them. The new patches have further 
>> tweaks and
>> should resolve the above issue.
>
> Thanks, but the first of the two patches lacks the commit log
> message.  And since you said there are further tweaks, I wasn't 
> sure
> the one from the previous version was still accurate.

Sorry about that. The workflow is still pretty foreign to me and
juggling with the old github upstream. I added the commit log.

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

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

Message #50 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: Sun, 12 Mar 2023 14:32:08 +0200

Wilhelm Kirschbaum <wkirschbaum <at> gmail.com> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
>>> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, 
>>> theo <at> thornhill.no
>>> Date: Sun, 12 Mar 2023 11:54:33 +0200
>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>>  >   In elixir-ts--call-parent-start:
>>> >   progmodes/elixir-ts-mode.el:459:38: Warning: Unused 
>>> >   lexical  >
>>> argument `node'
>>> >   progmodes/elixir-ts-mode.el:463:15: Warning: reference to 
>>> >   >
>>> free  >   variable `parent'
>>> >
>>> >   In elixir-ts--forward-sexp:
>>> >   progmodes/elixir-ts-mode.el:482:8: Warning: reference to 
>>> >   >
>>> free  >   variable `heex-ts--sexp-regexp'
>>> Ah, not sure how I missed them. The new patches have further 
>>> tweaks
>>> and
>>> should resolve the above issue.
>>
>> Thanks, but the first of the two patches lacks the commit log
>> message.  And since you said there are further tweaks, I wasn't 
>> sure
>> the one from the previous version was still accurate.
>
> Sorry about that. The workflow is still pretty foreign to me and
> juggling with the old github upstream. I added the commit log.
>
> [2. Add heex-ts-mode --- text/x-patch; 
> 0001-Add-heex-ts-mode-Bug-61996.patch]...
>
> [3. Add elixir-ts-mode --- text/x-patch; 
> 0002-Add-elixir-ts-mode-Bug-61996.patch]...

Please ignore the previous patch, need to fix some tests. 

Message #53 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: Sun, 12 Mar 2023 17:14:03 +0200

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

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

>> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
>> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
>> Date: Sun, 12 Mar 2023 11:54:33 +0200
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >   In elixir-ts--call-parent-start:
>> >   progmodes/elixir-ts-mode.el:459:38: Warning: Unused lexical 
>> >   argument `node'
>> >   progmodes/elixir-ts-mode.el:463:15: Warning: reference to 
>> >   free 
>> >   variable `parent'
>> >
>> >   In elixir-ts--forward-sexp:
>> >   progmodes/elixir-ts-mode.el:482:8: Warning: reference to 
>> >   free 
>> >   variable `heex-ts--sexp-regexp'
>> 
>> Ah, not sure how I missed them. The new patches have further 
>> tweaks and
>> should resolve the above issue.
>
> Thanks, but the first of the two patches lacks the commit log
> message.  And since you said there are further tweaks, I wasn't 
> sure
> the one from the previous version was still accurate.

Attached are the updated patches with added test cases and 
indentation
rule enhancements.

I still see this warning on a non-treesitter build:

In elixir-ts--forward-sexp:
elixir-ts-mode.el:490:8: Warning: reference to free variable
   ‘heex-ts--sexp-regexp’

But not sure why and how to fix it, because defcons 
heex-ts--sexp-regexp and
(require 'heex-ts-mode) is called.

Random concern: how will backwards compatibility work when the 
grammars
get updated?

It might just be better to add both modes in one patch if more 
changes
are required perhaps? 

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

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
Cc: 61996-done <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: Sun, 12 Mar 2023 17:46:41 +0200

> From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
> Cc: 61996 <at> debbugs.gnu.org, casouri <at> gmail.com, theo <at> thornhill.no
> Date: Sun, 12 Mar 2023 17:14:03 +0200
> 
> Attached are the updated patches with added test cases and
> indentation rule enhancements.

Thanks, installed on master.

> I still see this warning on a non-treesitter build:
> 
> In elixir-ts--forward-sexp:
> elixir-ts-mode.el:490:8: Warning: reference to free variable
>     ‘heex-ts--sexp-regexp’

Doesn't happen here, so I think we are good.

> Random concern: how will backwards compatibility work when the
> grammars get updated?

Let's discuss this when such problems actually happen.  The answer
depends on what kind of incompatibilities are introduced by changes in
the grammars.

> It might just be better to add both modes in one patch if more
> changes are required perhaps?

I'm not sure I understand the question.

In general, we like each commit to be as self-contained and
independent of the others as possible.  Not sure if this answers your
question.

Thanks, I'm closing this bug.

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

From: Wilhelm Kirschbaum <wkirschbaum <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61996-done <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: Sun, 12 Mar 2023 20:02:58 +0200

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

>> It might just be better to add both modes in one patch if more
>> changes are required perhaps?
>
> I'm not sure I understand the question.
>
> In general, we like each commit to be as self-contained and
> independent of the others as possible.  Not sure if this answers 
> your
> question.
>
> Thanks, I'm closing this bug.

Thanks for the help on this.

Previous Next

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