Subject: Re: bug#74461: [PATCH] Add go-work-ts-mode

From: Gabriel Santos <[email-masked]>
Date: Sun, 22 Dec 2024 08:32:39 -0300

>> --- a/lisp/progmodes/eglot.el
>> +++ b/lisp/progmodes/eglot.el
>> @@ -273,7 +273,7 @@ automatically)."
>>      (elm-mode . ("elm-language-server"))
>>      (mint-mode . ("mint" "ls"))
>>      ((kotlin-mode kotlin-ts-mode) . ("kotlin-language-server"))
>> -    ((go-mode go-dot-mod-mode go-dot-work-mode go-ts-mode go-mod-ts-mode)
>> +    ((go-mode go-dot-mod-mode go-dot-work-mode go-ts-mode go-mod-ts-mode go-work-ts-mode)
>>       . ("gopls"))
>>      ((R-mode ess-r-mode) . ("R" "--slave" "-e"
>>                              "languageserver::run()"))
>
>Do we have a way to tell Eglot which LSP server(s) to use via some
>buffer-local var, instead of having to change this centralized
>"database"?

Looking at the following commits:

- 5f56bc1 <https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=5f56bc1cdfcd474dd9cfad07240df6c252abd35c>
- e37754f <https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=e37754fc59bac409881d456a83aa0bf2468c94fb>

That doesn't seem to be the case.

>> @@ -565,6 +568,102 @@ what the parent of the node would be if it were a node."
>>  (if (treesit-ready-p 'gomod)
>>      (add-to-list 'auto-mode-alist '("/go\\.mod\\'" . go-mod-ts-mode)))
>>  
>> +;; go.work support.
>
>I'd use 3 or more semi-colons here, so it acts as a section separator.

I think 4 would be better in this case.
That would make these comments sub-sections
Of ";;; Code:"

>> +(defun go-work-ts-mode--in-directive-p ()
>> +  "Return non-nil if point is inside a Go workspace directive.
>
>This docstring doesn't seem right: the function returns another
>function, not a boolean.

Huh, that is true (it returns a lambda function).
Thankfully Randy is CC'ed here, so maybe
he can comment on that.

>> +        (pcase (treesit-node-type (treesit-node-at (point)))
>> +          ("replace" t)
>> +          ("use" t))))))
>
>AKA (member (treesit-node-type (treesit-node-at (point))) '("replace" "use"))

Thanks, It works. I'll also update the workspace function.

>> +;;;###autoload
>> +(define-derived-mode go-work-ts-mode prog-mode "Go Work"
>> +  "Major mode for editing go.work files, powered by tree-sitter."
>> +  :group 'go
>> +  :syntax-table go-work-ts-mode--syntax-table
>
>Why not use the standard name for the syntax-table (in which case you
>don't even need this `:syntax-table` argument)?

That's how it's done in the workspace
configuration,
but I removed it for work now
and testing by using treesit-explore
on the grammar's corpus shows nothing
out of the elements there.

Thanks again.

>> +    ;; Indent.
>> +    (setq-local indent-tabs-mode t
>> +                treesit-simple-indent-rules go-work-ts-mode--indent-rules)
>
>Is this `indent-tabs-mode` setting required by the definition of the
>go.work syntax/language, or is it a personal preference?  If it's
>a personal preference then it doesn't belong in the major mode, and if
>it's required by the syntax, then say so in a comment (ideally with
>a URL pointing to the relevant part of the language definition).

Go uses tabs for indentation,
and spaces for alignment.

This is valid even for workspace files,
as adding more than one package modifies
the use directive to multiple lines
indented with tabs.

<https://0x0.st/8r82.png>
(tabs added by running go work use, not by me)

<https://go.dev/doc/effective_go#formatting>

<https://pkg.go.dev/cmd/gofmt>

The language specification doesn't mention
that, but I think it should.
Maybe I should send them an e-mail about that.

<https://go.dev/ref/spec>

>> +(if (treesit-ready-p 'gowork)
>> +    (add-to-list 'auto-mode-alist '("/go\\.work\\'" . go-work-ts-mode)))
>
>Since we don't have another (non-treesitter) mode for these files, I'd
>recommend you go straight for:
>
>    ;;;###autoload
>    (add-to-list 'auto-mode-alist '("/go\\.work\\'" . go-work-ts-mode))

Sorry if I misunderstand,
but that doesn't seem to be the case
for other tree-sitter modes.

See rust-ts-mode for instance:

<https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/rust-ts-mode.el?id=59367f6a3a9dd7fb30429494b622ebdec94e6e32>

The non-treesitter mode for this language is an external package.

And Elixir too:

<https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/elixir-ts-mode.el?id=59367f6a3a9dd7fb30429494b622ebdec94e6e32>

Seems like we need further conversation on this topic.

I'll make the changes you requested and send the updated patch after I'm done.

--
Gabriel Santos