search

fwcd / kotlin-language-server

Kotlin code completion, diagnostics and more for any editor/IDE using the Language Server Protocol
MIT License
1.62k stars 203 forks source link

Infix code completion #521

Closed elamc-2 closed 9 months ago

elamc-2 commented 10 months ago

Closes #446

Adds missing completions for infix functions.

fwcd commented 10 months ago

Neat, thanks a lot for looking into this! Bit short on time, so I won't review it in detail right now, but it looks pretty good from a first glance (thanks for adding tests too).

elamc-2 commented 10 months ago

Tested it manually as well, and works great 🙂 Good job! 🎉

The only little case that is not working is when completing nothing (i.e, 1 instead of 1 a, where the last gives us and as an alternative). The server is not generally good at handling these cases, so think we might need bigger rewrites to make it work. Maybe we should consider it future work and make an issue for it? I have been missing global completion with EVERYTHING when completing nothing, but unsure if it is just me or not...

Added global completions @themkat let me know how it looks. Detekt isn't agreeing with these changes so will need to look into that. Also could do with some work regarding performance?

themkat commented 9 months ago

Tested it manually as well, and works great 🙂 Good job! 🎉 The only little case that is not working is when completing nothing (i.e, 1 instead of 1 a, where the last gives us and as an alternative). The server is not generally good at handling these cases, so think we might need bigger rewrites to make it work. Maybe we should consider it future work and make an issue for it? I have been missing global completion with EVERYTHING when completing nothing, but unsure if it is just me or not...

Added global completions @themkat let me know how it looks. Detekt isn't agreeing with these changes so will need to look into that. Also could do with some work regarding performance?

Sorry for the late reply, will try to have a closer look tomorrow 🙂 Been busy with some other things... 🙁

Looking at the detekt violations, it seems to me that those are because of the existing implementation more than what you have done. I would say that you use @Suppress with those methods for now (e.g, @Suppress("ReturnCount")). Sounds okay to you? Will let you know if I find some obvious fixes to the Detekt issues.

themkat commented 9 months ago

It's been 2 days, so merging now. Thank you @ElamC 😄