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

Avoid mutating completion item in resolve #2009

Merged
merged 1 commit into from May 8, 2024

Conversation

vinistock
Copy link
Member

Motivation

Closes #1983

Like the author of the issue, I had also noticed that we stopped using short names for constants in completion. Which puzzled me a lot given that we explicitly handle that and have unit tests to verify the behaviour.

After reading the specification again, I finally realized the issue.

When we split completion into completion + resolve, there was a mistake in the original PR. The resolve request should never mutate any existing fields of the completion item other than label details and documentation. But instead, we were creating a new completion item from scratch without including all of the original fields.

That means that the insertText we were specifying in completion was getting erased during resolve, which forces the editor to use the label instead, resulting in the behaviour of showing the fully qualified name.

Implementation

We never want to mutate anything in resolve that isn't label details and documentation. We should take the original item as a hash, change only those fields and then return it.

This fixes the issue of using the wrong sorting text and the wrong insertion text, which we're currently seeing in main.

Automated Tests

Improved the resolve test a bit.

@vinistock vinistock added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels May 3, 2024
@vinistock vinistock self-assigned this May 3, 2024
@vinistock vinistock requested a review from a team as a code owner May 3, 2024 19:28
@vinistock vinistock requested review from andyw8 and st0012 May 3, 2024 19:28
@vinistock vinistock force-pushed the vs/avoid_modifying_original_completion_item branch from f7bfaf4 to 8a26a41 Compare May 8, 2024 16:19
@vinistock vinistock enabled auto-merge (squash) May 8, 2024 16:19
@vinistock vinistock merged commit f7acc5b into main May 8, 2024
39 checks passed
@vinistock vinistock deleted the vs/avoid_modifying_original_completion_item branch May 8, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't suggest fully qualified constants when unnecessary
2 participants