Hi,

Thanks for the review, my comments are below.

Commit with changes:
https://git.sr.ht/~akagi/devicetree-ts-mode/commit/bc07c1124545cbf6e5ebe64e92bfaa306e309033

On 2023-12-22 07:36, Philip Kaludercic wrote:

> Aleksandr Vityazev <avityazew@gmail.com> writes:
>
>> Hello,
>>
>> I'd like to submit devicetree-ts-mode [1] to GNU ELPA.
>> This is tree-sitter major mode for Devicetree [2] files.
>>
>> [1] https://git.sr.ht/~akagi/devicetree-ts-mode/
>> [2] https://www.devicetree.org/
>
> Here are a few comments:
>
> diff --git a/devicetree-ts-mode.el b/devicetree-ts-mode.el
> index 1d9f72c..d26937c 100644
> --- a/devicetree-ts-mode.el
> +++ b/devicetree-ts-mode.el
> @@ -33,6 +33,7 @@
>  ;; * IMenu
>  ;; * Font Lock
>  
> +;; The commentary section could elaborate on what "Devicetree" are.

Fixed.

>  ;;; Code:
>  
> @@ -44,15 +45,21 @@
>  (declare-function treesit-parser-create "treesit.c")
>  (declare-function treesit-node-child-by-field-name "treesit.c")
>  
> +(defgroup devicetree ()
> +  "Tree-sitter support for DTS."
> +  :prefix "devicetree-ts-"
> +  :group 'languages)
> +

Applied.

>  (defcustom devicetree-ts-mode-indent-offset 4
>    "Number of spaces for each indentation step in `devicetree-ts-mode'."
>    :version "29.1"
> +  ;; This is not a core package, the version of your package is 0.2,
> +  ;; so this doesn't match up
Fixed.
>    :type 'natnum
> -  :safe 'natnump
> -  :group 'devicetree)
> +  :safe 'natnump)
Applied.  
>  ;; Taken from the dts-mode
> -(defvar devicetree-ts-mode--syntax-table
> +(defvar devicetree-ts-mode-syntax-table
Applied.  
>    (let ((table (make-syntax-table)))
>  
>      (modify-syntax-entry ?<  "(>" table)
> @@ -81,6 +88,10 @@
>  
>  (defvar devicetree-ts-mode--indent-rules
>    (let ((offset devicetree-ts-mode-indent-offset))
> +    ;; If this is a variable, that is set when the package is loaded,
> +    ;; customising the user option `devicetree-ts-mode-indent-offset'
> +    ;; will have no effect.  You could turn this into a function
> +    ;; instead.
>      `((devicetree
>         ((node-is ">") parent-bol 0)
>         ((node-is "]") parent-bol 0)
> @@ -93,13 +104,14 @@
>         (no-node parent-bol 0))))
>    "Tree-sitter indent rules for `devicetree-ts-mode'.")
Replaced by function.

> +;; Could these be defconst?
>  (defvar devicetree-ts-mode--treesit-keywords
>    '("/delete-node/" "/delete-property/" "#define" "#include"
>      "/omit-if-no-ref/" "/dts-v1/"))
>  
>  (defvar devicetree-ts-mode--treesit-operators
> -  '( "!" "~" "-" "+" "*" "/" "%" "||" "&&" "|"
> -     "^" "&" "==" "!=" ">" ">=" "<=" ">" "<<" ">>"))
> +  '("!" "~" "-" "+" "*" "/" "%" "||" "&&" "|"
> +    "^" "&" "==" "!=" ">" ">=" "<=" ">" "<<" ">>"))
>
Replaced defvar with defconst.

>  (defvar devicetree-ts-mode--font-lock-settings
>    (treesit-font-lock-rules
> @@ -154,8 +166,8 @@
>  
>  (defun devicetree-ts-mode--node-addresses (node)
>    "List of addresses for NODE."
> -  (reverse
> -   (seq-reduce
> +  (reverse				;Why `reverse'? (or `nreverse'?)
> +   (seq-reduce				;Isn't this a `seq-filter'?
>      (lambda (acc children)
>        (if (string-equal (treesit-node-field-name children)
>                          "address")

seq-filter returns the element itself, so the code would look like this

#+begin_src elisp 
  (defun devicetree-ts-mode--node-addresses (node)
  "List of addresses for NODE."
  (let ((address-nodes
         (seq-filter
          (lambda (children)
            (when (string-equal (treesit-node-field-name children)
                                "address")
              t))
          (treesit-node-children node))))
    (seq-map (lambda (children)
               (treesit-node-text children t))
             address-nodes)))

#+end_src

I think the code with reverse looks better, so I left it as it was.

> @@ -175,8 +187,6 @@
>  ;;;###autoload
>  (define-derived-mode devicetree-ts-mode prog-mode "DTS"
>    "Major mode for editing devicetree, powered by tree-sitter."
> -  :group 'devicetree
> -  :syntax-table devicetree-ts-mode--syntax-table

Applied.  
>    (when (treesit-ready-p 'devicetree)
>      (treesit-parser-create 'devicetree)
> @@ -187,7 +197,7 @@
>  
>      ;; Imenu.
>      (setq-local treesit-simple-imenu-settings
> -                `((nil "\\`node\\'"
> +                `((nil ,(rx bos "node" eos)
>                         nil devicetree-ts--mode--name-function)))

Applied.  
>      (setq-local which-func-functions nil)
>  
> @@ -216,9 +226,8 @@
>  
>      (treesit-major-mode-setup)))
>  
> -(if (treesit-ready-p 'devicetree)
> -    (add-to-list 'auto-mode-alist
> -                 '("\\.dtsi?\\'" . devicetree-ts-mode)))
> +(when (treesit-ready-p 'devicetree)
> +  (add-to-list 'auto-mode-alist '("\\.dtsi?\\'" . devicetree-ts-mode)))
Applied.

>  (provide 'devicetree-ts-mode)
>  ;;; devicetree-ts-mode.el ends here
>
>
>>
>> From b69687be8232cfd305893b7c0b6999e6667d6dd8 Mon Sep 17 00:00:00 2001
>> Message-ID: <b69687be8232cfd305893b7c0b6999e6667d6dd8.1703075351.git.avityazew@gmail.com>
>> From: Aleksandr Vityazev <avityazew@gmail.com>
>> Date: Wed, 20 Dec 2023 15:28:35 +0300
>> Subject: [PATCH] * elpa-packages (devicetree-ts-mode): New package
>>
>> ---
>>  elpa-packages | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/elpa-packages b/elpa-packages
>> index 612bc676cd..6f86a59cfd 100644
>> --- a/elpa-packages
>> +++ b/elpa-packages
>> @@ -207,6 +207,8 @@
>>    :news "CHANGELOG.org"
>>    :readme "README.md")
>>   (devdocs		:url "https://github.com/astoff/devdocs.el")
>> + (devicetree-ts-mode	:url "https://sr.ht/~akagi/devicetree-ts-mode"
>> +  :ignored-files ("LICENSE"))
>
> You can track this file in an .elpaignore file, within your repository
> (which you appear to have anyway).

New patch attached.
>
>>   (dict-tree		:url nil) ;"http://www.dr-qubit.org/git/predictive.git"
>>   (diff-hl		:url "https://github.com/dgutov/diff-hl.git")
>>   (diffview            	:url "https://github.com/mgalgs/diffview-mode.git")
>>
>> base-commit: b7bbd439862f2a58151eacacebc1815b7ddf3322
>> -- 
>> 2.41.0
>