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

Add support to filter chain by passing the in the token #1282

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

gayanper
Copy link
Contributor

@gayanper gayanper commented Mar 24, 2024

What it does

Add a new constructor to pass in the token so that the token will be used when matching the chains. This helps to better control how much chains are produced by using the text that is already typed in an editor as the token. This helps to provide better chains selection without increasing the number of chains to produce for JDT.LS. The existing constructor will preserve the current behavior uneffected.

How to test

This new behavior can be only tested with JDT.LS PR eclipse-jdtls/eclipse.jdt.ls#2835

Author checklist

@gayanper
Copy link
Contributor Author

gayanper commented Apr 7, 2024

@rgrunber can you review this and approve ?

@rgrunber rgrunber added this to the 4.32 M2 milestone Apr 8, 2024
@gayanper gayanper requested a review from jjohnstn April 26, 2024 04:21
@gayanper
Copy link
Contributor Author

@jjohnstn @rgrunber can one of you give me a go ahead for this. This is to actually improve the jdtls chain completion functionality

@jjohnstn jjohnstn modified the milestones: 4.32 M2, 4.32 M3 Apr 26, 2024
Copy link
Contributor

@jjohnstn jjohnstn left a comment

Choose a reason for hiding this comment

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

I have no issues with the changes, but I don't work with jdt.ls so have no way of verifying this; therefore I'll have to defer to @rgrunber to at least test and report back. I have moved to 4.32 M3 as 4.32 M2 has sailed. Is this something that JDT UI users would also want to utilize in the future?

@gayanper
Copy link
Contributor Author

I have no issues with the changes, but I don't work with jdt.ls so have no way of verifying this; therefore I'll have to defer to @rgrunber to at least test and report back.

We tried the out in the following MR in jdt.ls eclipse-jdtls/eclipse.jdt.ls#2835. But I will wait for @rgrunber

I have moved to 4.32 M3 as 4.32 M2 has sailed. Is this something that JDT UI users would also want to utilize in the future?

Yes I think JDT UI can use this as well if it is a good idea.

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I'm fine with merging for M3. If it's an improvement, we can write testcases once/if JDT UI adopts it.

@jjohnstn jjohnstn merged commit eae41a0 into eclipse-jdt:master Apr 30, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants