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

From: john muhl <[email-masked]>
Date: Sat, 9 Sep 2023 10:17:20 -0500

Mauro Aranda <[email-masked]> writes:

> I just have some comments/questions about the defcustoms:

Fixed.

Philip Kaludercic <[email-masked]> writes:

> ~johnmuhl <[email-masked]> writes:
>
>> +(defcustom lua-ts-lua-manual
>> +  (if (file-readable-p "/usr/share/doc/lua/manual.html")
>> +      "file:///usr/share/doc/lua/manual.html" "")
>> +  "Location of the Lua `manual.html' file."
>> +  :type 'string
>> +  :safe 'stringp
>> +  :group 'lua
>> +  :version "30.1")
>
> Could this fall back to some online manual?

I went with Augusto’s suggestion and removed the documentation
command.

>> +(defcustom inferior-lua-interpreter "lua"
>> +  "Program to run in the inferior Lua process."
>> +  :type 'string
>> +  :safe 'stringp
>> +  :group 'lua
>> +  :version "30.1")
>
> Are you sure that any string is safe?  That would include "rm -rf ~".

Fixed here and for the startfile and command line options.

>> +;;;###autoload
>> +(define-derived-mode lua-ts-mode prog-mode "Lua"
>> +  "Major mode for editing Lua files, powered by tree-sitter."
>> +  :group 'lua
>
> The :group is redundant here, it will automatically use the last
> defgroup defined in the file.

Fixed.

Augusto Stoffel <[email-masked]> writes:

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

Fixed.

> Also, I think it's probably best to use the same lua-ts- prefix

Changed to lua-ts-inferior-*

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

Removed.

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

I like the way o-m-m folds the header sections of elisp files and
since my lua files often start with a multi-line comment containing
similar information I added it to get that effect in lua too.

> Should one allow whitespace in front of these strings (also inferring
> the outline level from that)?

Done.

> Also, one should allow arbitrary whitespace after "local" (I'd say
> "local\\s-+") and enclose the keywords with "\\_<...\\_>".

Fixed.