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

fix duplicate results when search for * #2547

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

CsCherrYY
Copy link
Contributor

fix #2938

Signed-off-by: Shi Chen <chenshi@microsoft.com>
@CsCherrYY CsCherrYY added the bug label Mar 20, 2023
@CsCherrYY CsCherrYY marked this pull request as ready for review March 21, 2023 05:48
@CsCherrYY CsCherrYY requested a review from rgrunber March 21, 2023 05:48
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.

I think if we really need to fix it this way, we could but there's one regression that might be introduced. The problem is you can't really see it on the VS Code side because of microsoft/vscode#149144 .

Something like java.util.function.* returns results from the LS, but VS Code would fail to display anything. With this change, the above query returns empty results in the LS also. To me, the query should return something in an ideal world 😛 .

Is there any reason this issue can't be solved by just making a temporary symbol set, and then converting to a symbol list (to satisfy the API)

@CsCherrYY
Copy link
Contributor Author

@rgrunber you're right. The current fix will return an empty array. Why I didn't use a temporary set is I'd like to reduce an unnecessary search, but it's now tricky to set the condition to "exclude" the search.

I would update this commit in the set/list way.

Signed-off-by: Shi Chen <chenshi@microsoft.com>
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.

LGTM. I would just confirm that test failure in WorkspaceSymbolHandlerTest was just a random failure. Couldn't reproduce locally. Feel free to squash+merge after.

@rgrunber
Copy link
Contributor

rgrunber commented Mar 22, 2023

I think this cause some issue in https://ci.eclipse.org/ls/job/jdt-ls-master/1625/testReport/ . at WorkspaceSymbolHandlerTest.testProjectSearch .

@CsCherrYY
Copy link
Contributor Author

I think this cause some issue in https://ci.eclipse.org/ls/job/jdt-ls-master/1625/testReport/ . at WorkspaceSymbolHandlerTest.testProjectSearch .

I can't reproduce it locally. Is it related to the platform?

@rgrunber
Copy link
Contributor

rgrunber commented Mar 22, 2023

I wasn't able to reproduce in an IDE, but if I run with

$ mvn clean verify -DtestClass=org.eclipse.jdt.ls.core.internal.handlers.WorkspaceSymbolHandlerTest -DfailIfNoTests=false
Failures: 
  WorkspaceSymbolHandlerTest.testProjectSearch:119 expected:<[java]> but was:<[org.sample]>

Tests run: 17, Failures: 1, Errors: 0, Skipped: 0

Update: : I see the test does a search and gets the first entry .. maybe we've disturbed the ordering. Might be a simple fix to make the test more resilient.

It's an easy fix. There are java.IFoo & org.sample.IFoo . We get the first entry and assume it'll be the java package but we make no guarantees about ordering at this point so either is valid. I'll just fix the test to check for both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show the new class name repeatedly when moving class
2 participants