all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Wilhelm Kirschbaum <wkirschbaum@gmail.com>
Cc: 61996@debbugs.gnu.org, theo@thornhill.no, casouri@gmail.com
Subject: bug#61996: 30.0.50; Submitting elixir-ts-mode and heex-ts-mode
Date: Sat, 11 Mar 2023 11:16:08 +0200	[thread overview]
Message-ID: <83356by7fr.fsf@gnu.org> (raw)
In-Reply-To: <877cvtfz37.fsf@gmail.com> (message from Wilhelm Kirschbaum on Mon, 06 Mar 2023 21:24:11 +0200)

> From: Wilhelm Kirschbaum <wkirschbaum@gmail.com>
> Cc: 61996@debbugs.gnu.org, casouri@gmail.com, theo@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@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@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.





  parent reply	other threads:[~2023-03-11  9:16 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-06  7:04 bug#61996: 30.0.50; Submitting elixir-ts-mode and heex-ts-mode Wilhelm Kirschbaum
2023-03-06 11:59 ` Eli Zaretskii
2023-03-06 17:23   ` Wilhelm Kirschbaum
2023-03-06 18:36     ` Eli Zaretskii
2023-03-06 19:24       ` Wilhelm Kirschbaum
2023-03-06 20:14         ` Eli Zaretskii
2023-03-11  9:16         ` Eli Zaretskii [this message]
2023-03-11 14:16           ` Dmitry Gutov
2023-03-11 18:27             ` Wilhelm Kirschbaum
2023-03-11 18:01           ` Wilhelm Kirschbaum
2023-03-12  9:00             ` Eli Zaretskii
2023-03-12  9:54               ` Wilhelm Kirschbaum
2023-03-12 11:37                 ` Eli Zaretskii
2023-03-12 12:23                   ` Wilhelm Kirschbaum
2023-03-12 12:32                     ` Wilhelm Kirschbaum
2023-03-12 15:14                   ` Wilhelm Kirschbaum
2023-03-12 15:46                     ` Eli Zaretskii
2023-03-12 18:02                       ` Wilhelm Kirschbaum
2023-03-06 16:41 ` Dmitry Gutov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83356by7fr.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=61996@debbugs.gnu.org \
    --cc=casouri@gmail.com \
    --cc=theo@thornhill.no \
    --cc=wkirschbaum@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.