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

lsp: Rename correctly when conflicting #1334

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

charlieegan3
Copy link
Member

This updates the server's rename quick action to use the same name generation conflict resolution logic as the fixer.

This is done by allowing the server's cache to be used as a new file provider and consolidating the logic for generating changes required for the templating worker and command worker. FixViolations is also added to support more targetted fixes.

This also changes the inlay hints to exit early if the file is not parsed. This reduces errors in the server logs when working with empty files.

@charlieegan3 charlieegan3 force-pushed the rename-conflict-lsp-2 branch from a217da3 to 9c948d5 Compare January 14, 2025 10:24
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Left a couple of suggestions / questions, but beside that, LGTM!

One last thing — do we have existing tests that cover moving a file and applying another fix, like no-whitespace-comment? I know we had some issues with that in the past, as the other linters would not get notified of the rename. If we don't have that covered, it'd be good to test for!

internal/lsp/cache/cache.go Show resolved Hide resolved
internal/lsp/server.go Outdated Show resolved Hide resolved
pkg/fixer/fileprovider/cache.go Show resolved Hide resolved
pkg/fixer/fileprovider/cache.go Outdated Show resolved Hide resolved
@charlieegan3
Copy link
Member Author

do we have existing tests that cover moving a file and applying another fix

I thought we did, but I have updated a fixer test now to cover this case in:

5a52439

@charlieegan3 charlieegan3 force-pushed the rename-conflict-lsp-2 branch 2 times, most recently from 8ce3897 to 2bc0e1d Compare January 15, 2025 14:15
This updates the server's rename quick action to use the same name
generation conflict resolution logic as the fixer.

This is done by allowing the server's cache to be used as a new file
provider and consolidating the logic for generating changes required for
the templating worker and command worker.

This also changes the inlay hints to exit early if the file is not
parsed. This reduces errors in the server logs when working with empty
files.

Signed-off-by: Charlie Egan <[email protected]>
This adapts the LSP and fixer to use the versionsMap functionality in
the same way as the linter.

Signed-off-by: Charlie Egan <[email protected]>
@charlieegan3 charlieegan3 force-pushed the rename-conflict-lsp-2 branch from 2bc0e1d to 864a51b Compare January 15, 2025 14:15
@charlieegan3 charlieegan3 merged commit faa9c52 into StyraInc:main Jan 15, 2025
5 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.

2 participants