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

Force public APIs to return complete results #68804

Merged
merged 2 commits into from
Dec 27, 2023

Conversation

sharwell
Copy link
Member

External consumers of CompletionService (for unit testing CompletionProvider implementations) don't have access to the waiter APIs necessary to use non-blocking completion in testing scenarios. These APIs are updated to adhere to their behavior prior to #64352 introducing non-deterministic results.

External consumers of CompletionService (for unit testing
CompletionProvider implementations) don't have access to the waiter
APIs necessary to use non-blocking completion in testing scenarios.
These APIs are updated to adhere to their behavior prior to dotnet#64352
introducing non-deterministic results.
@sharwell sharwell requested a review from a team as a code owner June 27, 2023 15:28
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 27, 2023
@CyrusNajmabadi
Copy link
Member

Just to verify though. Existing callers in Vs/vscode will still benefit here and can then the paths that may produce incomplete results faster, right?

@sharwell
Copy link
Member Author

sharwell commented Jun 27, 2023

@CyrusNajmabadi Existing behavior for VS scenario does not change. Whether VS Code uses the old path or the new path is unclear, but in either case it would be acceptable.

// https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1620947
if (options.ForceExpandedCompletionIndexCreation)
Copy link
Member

Choose a reason for hiding this comment

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

The fix looks good to me. But I'd suggest to change the name of this option to be more general to reflect this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ I left the option name the same, but updated the comment

@genlu
Copy link
Member

genlu commented Jul 24, 2023

Whether VS Code uses the old path or the new path is unclear, but in either case it would be acceptable.

Our LSP handler uses the internal API, so it's not affected by this (i.e. not waiting for provider from reference to be loaded before returning)

@sharwell sharwell merged commit ffea8b3 into dotnet:main Dec 27, 2023
28 checks passed
@sharwell sharwell deleted the non-responsive-load branch December 27, 2023 19:32
@ghost ghost added this to the Next milestone Dec 27, 2023
@Cosifne Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants