Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infix code completion #521

Merged
merged 3 commits into from
Nov 15, 2023
Merged

Infix code completion #521

merged 3 commits into from
Nov 15, 2023

Conversation

elamc-2
Copy link
Contributor

@elamc-2 elamc-2 commented Oct 22, 2023

Closes #446

Adds missing completions for infix functions.

@fwcd fwcd added enhancement New feature or request code completion Auto completion labels Oct 22, 2023
@fwcd
Copy link
Owner

fwcd commented Oct 22, 2023

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

Copy link
Collaborator

@themkat themkat left a comment

Choose a reason for hiding this comment

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

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

@github-actions github-actions bot added the code quality Refactoring, tests etc. label Nov 6, 2023
@elamc-2
Copy link
Contributor Author

elamc-2 commented Nov 6, 2023

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?

@elamc-2 elamc-2 requested a review from themkat November 9, 2023 13:29
@themkat
Copy link
Collaborator

themkat commented Nov 12, 2023

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.

@elamc-2 elamc-2 changed the title Code completion when calling functions using infix notation Infix code completion Nov 13, 2023
Copy link
Collaborator

@themkat themkat left a comment

Choose a reason for hiding this comment

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

Can't see any obvious faults! Good job! Completing infix functions works amazing, especially globally! Thank you for amazing work! ❤️ Very happy to see that your changes are verified by both your own and existing tests.

Sorry if I'm a downer with this last sentence... but.. I'm going to give @fwcd a day or two to have a look before merging. Like to give him a chance to have a look before merging, especially since he have his own set of opinions on code style etc. 🙂 (FWs repo after all)

@themkat
Copy link
Collaborator

themkat commented Nov 15, 2023

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

@themkat themkat merged commit bd76446 into fwcd:main Nov 15, 2023
8 checks passed
@elamc-2 elamc-2 deleted the infix-completions branch November 15, 2023 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code completion Auto completion code quality Refactoring, tests etc. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code completion when typing infix method
3 participants