bug#61996: 30.0.50; Submitting elixir-ts-mode and heex-ts-mode

[Wilhelm Kirschbaum]

Eli Zaretskii <eliz@gnu.org> writes:

> > >From 88c941067da0e34e1e9ababeb813ba51378ae2cc Mon Sep 17 
> > >00:00:00 2001
> > From: Wilhelm H Kirschbaum <wkirschbaum@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@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
Description: Add heex-ts-mode

0002-Add-elixir-ts-mode-Bug-61996.patch
Description: Add elixir-ts-mode