Subject: Re: bug#65672: [PATCH emacs 1/1] Add lua-ts-mode

From: Augusto Stoffel <[email-masked]>
Date: Sat, 02 Sep 2023 16:10:06 +0200

On Thu, 31 Aug 2023 at 16:32, ~johnmuhl wrote:

> +(defcustom inferior-lua-switches "-i"
> +  "Command line options for the inferior Lua process."
> +  :type 'string
> +  :safe 'stringp
> +  :group 'lua
> +  :version "30.1")

This should be given as a list of strings rather a string that you later
need to call split-string-shell-command on.

Also, I think it's probably best to use the same lua-ts- prefix for the
Lua shell variables and command.  If you're really fond of a
`inferior-lua' or `run-lua' command, it could be an alias (but then one
might need to think about what to do about name clashes when one of the
other Lua modes is installed).

> +(defcustom inferior-lua-startfile ""
> +  "File to load into the inferior Lua process at startup."
> +  :type 'string
> +  :safe 'stringp
> +  :group 'lua
> +  :version "30.1")

This should be nil by default.

> +(defun lua-ts-documentation-at-point ()
> +  "Show documentation of function at point in Lua manual."
> +  (interactive)
> +  (unless (string-blank-p lua-ts-lua-manual)
> +    (let ((character-before (char-to-string (char-before)))
> +          id)
> +      (save-excursion
> +        ;; When point is mid-word `treesit-thing-at-point'
> +        ;; may return the parent node of the thing at point.
> +        (unless (or (bolp)
> +                    (not (string-match-p "[[:alnum:]]" character-before)))
> +          (backward-word))
> +        (let ((node (treesit-thing-at-point 'builtin nil)))
> +          (setq id (pcase (treesit-node-type node)
> +                     ("dot_index_expression" (treesit-node-text node t))
> +                     ("function_call"
> +                      (let* ((child (treesit-node-child-by-field-name node "name"))
> +                             (name (treesit-node-text child t)))
> +                        (if (string-match-p ":" name)
> +                            (replace-regexp-in-string "^.*:" "file:" name)
> +                          name)))))))
> +      (when id (browse-url (concat lua-ts-lua-manual "#pdf-" id))))))

I wouldn't add this command.  It's not polished enough and too ad-hoc in
the sense that this functionality is (or should be) covered by other
mechanisms: Info, Eldoc, etc.

> +    ;; Outline.
> +    (setq-local outline-regexp
> +                (rx (or "--[[" "do" "for" "if" "repeat" "while"
> +                        (seq (** 0 1 "local ") "function"))))

What is the idea behind "--[["?  Should one allow whitespace in front of
these strings (also inferring the outline level from that)?  Also, one
should allow arbitrary whitespace after "local" (I'd say "local\\s-+")
and enclose the keywords with "\\_<...\\_>".

In general, this looks nice, thanks!