godotengine / emacs-gdscript-mode

An Emacs package to get GDScript support and syntax highlighting.
GNU General Public License v3.0
316 stars 36 forks source link

Added integration with eglot #122

Closed RuijieYu closed 1 year ago

RuijieYu commented 1 year ago

Fixes #121.

RuijieYu commented 1 year ago

I don't have the proper environment for this, @xiliuya (or someone else), can you test this PR and see if it works?

I'm unsure about the header. It seems that there is an "author" field, but I don't know if that is reserved for the entire repository or for authoring each file.

This PR also took inspiration from code by @xiliuya, and I'm wondering whether/how I should properly attribute that.

joaotavora commented 1 year ago

I think you should also add something to gdscript-mode.el that require's this file, checks if the eglot-server-programs variable exists and uses add-to-list to add the new function to that variable.

RuijieYu commented 1 year ago

I think you should also add something to gdscript-mode.el that require's this file, checks if the eglot-server-programs variable exists and uses add-to-list to add the new function to that variable.

I added the require expr in gdscript-mode. Also added a push that adds gdscript to eglot-server-programs as soon as eglot is loaded. I think that's enough? I didn't put a lambda wrapping gdscript-eglot-contact because that function is autoloaded from the user session, and also required from within the -mode file.

NathanLovato commented 1 year ago

Thanks much for taking the time to make this contribution.

Note that I I'm not maintaining this module anymore, I don't use Emacs anymore, so this project needs a new maintainer who actively uses Emacs to review, test, and merge your changes.

xiliuya commented 1 year ago

I don't have the proper environment for this, @xiliuya (or someone else), can you test this PR and see if it works?

I'm unsure about the header. It seems that there is an "author" field, but I don't know if that is reserved for the entire repository or for authoring each file.

This PR also took inspiration from code by @xiliuya, and I'm wondering whether/how I should properly attribute that.

Great, I don't need to change the configuration file.

RuijieYu commented 1 year ago

@NathanLovato Given the situation, do you want to make a pinned issue on this repository to look for someone who wants to take over the maintainership? I have never used gdscript nor this code, so I definitely wouldn't qualify. Unless @xiliuya wants to take over the maintainership for this repo, of course.

xiliuya commented 1 year ago

@RuijieYu Yes, I want to become the maintainer of this repository. Now I have replied in relevant issues/123

xiliuya commented 1 year ago

Hello, @RuijieYu , Thank you very much for your submission. I found the following problems in my test, which need your modification:

xiliuya commented 1 year ago

@RuijieYu
gdscript-eglot.el need commentary. As described below:

;;; Commentary:
;;
;; Handles the configuration of eglot.
;; It supports gdscript-mode using eglot.
;;
RuijieYu commented 1 year ago

Fixed. Wasn't sure what I was thinking back then about push eglot...

xiliuya commented 1 year ago

@NathanLovato This PR is ready to merge into master.

RuijieYu commented 1 year ago

Almost forgot that I had that TODO there. Also, fixed a docstring inconsistency (used to say INTERACTIVE-P but should be INTERACTIVE instead). Now I think it is ready.

RuijieYu commented 1 year ago

Seems like something is unhappy about eask on Windows. Is it due to the PR, or something else?

xiliuya commented 1 year ago

@RuijieYu I think it's will fix with #118

NathanLovato commented 1 year ago

Thank you for the contribution! The windows-latest test doesn't pass currently but it's just incompatible with eask it seems. As mentioned in #118 a solution is to add an experimental flag to bypass this (as it is for an experimental Emacs version, hence the incompatibility).

So as review passed here, I'm merging. Thank you very much for your work, you two!