Skip to content

Conversation

@themkat
Copy link
Contributor

@themkat themkat commented Sep 13, 2022

Functionality to support override/implement member was recently merged in the language server (screenshot in that PR):
fwcd/kotlin-language-server#379

Think I described the functionality quite well in the PR above:

Override any member that is currently not implemented in the current class. This includes open functions, functions with default implementations, open members, abstract stuff etc. The main difference here is that this is general override member functionality, NOT JUST abstract members like the quick fix does. This is super useful for cases where you need to override a default implementation (e.g toString, Thread run, lsp4j interface methods like in TextDocumentService etc.), NOT just the unimplemented abstract ones. For me it would be super useful for my day job using Kotlin 😄 Having to google api docs to find signatures are a pain in the ass just for doing simple stuff like this.

(the quick fix I'm referring to is a code action that the language server provides)

I think an interactive function makes sense here. The users can then create a key binding for it if they want to. If you feel like we should have one out of the box, let me know 🙂

This functionality is very similar to the Java-version of implement/override methods, but has a few minor differences in how it works. Both need a multiple select field where we can select the methods/members we want. Due to this reason I stole that function from lsp-java:
https://github.com/emacs-lsp/lsp-java/blob/a1aff851bcf4f397f2a968557d213db1fede0c8a/lsp-java.el#L1065

Maybe it should be in a common place? At this point I don't know where it should really be.

@github-actions github-actions bot added the client One or more of lsp-mode language clients label Sep 13, 2022
Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

(lambda (member-options)
(-if-let* ((option-items (-map (lambda (x)
(list (lsp-get x :title)
(ht-get (lsp-get (lsp-get x :edit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ht-get -> lsp-get

Copy link
Contributor Author

@themkat themkat Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with that is that lsp-get seems to expect a symbol as the key, while ht-get allows strings as well ((lsp--buffer-uri) is a string). I get the following error when using lsp-get in place of ht-get here:
Error processing message (wrong-type-argument symbolp "file:///....")

Tried a few other solutions as well, like converting to symbol, but I couldn't get it to work properly. Let me know if there is something I'm missing 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try something like this:

(let ((key "uri"))
  (intern (concat ":" key) ))

The issue is that when you run that code with lsp-use-plists that map wont the hashtable but it will be plists and it won't work.

Copy link
Contributor Author

@themkat themkat Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works! Thought I tried that earlier... Damn brain 😆

Also good to know about that lsp-use-plists-stuff! Will try to remember that for future work.

@yyoncho yyoncho merged commit 9cb2cb3 into emacs-lsp:master Sep 13, 2022
@yyoncho
Copy link
Member

yyoncho commented Sep 13, 2022

Thank you! Not that happy with having that multiselect here but we are pretty much out of options...

@themkat themkat deleted the kotlin_overrides branch September 13, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client One or more of lsp-mode language clients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants