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

Implements Rename provider #314

Merged
merged 8 commits into from
Oct 29, 2024
Merged

Implements Rename provider #314

merged 8 commits into from
Oct 29, 2024

Conversation

simerplaha
Copy link
Member

@simerplaha simerplaha commented Oct 15, 2024

@simerplaha simerplaha requested a review from tdroxler October 15, 2024 23:22
@simerplaha simerplaha changed the base branch from master to distinct_references October 16, 2024 00:48
/**
* Result types for GoTo references location search results.
*/
sealed trait GoToRef extends GoTo
sealed trait GoToRef extends Rename
Copy link
Member

Choose a reason for hiding this comment

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

it sounds weird that GoToRefextends Rename, is it expected?

Copy link
Member Author

@simerplaha simerplaha Oct 16, 2024

Choose a reason for hiding this comment

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

Good catch. The only reason for this is to save a transformation because rename returns the same result type as GoToRef. Having GoToRef as a subtype of Rename means we can return the GoToRef results from rename immediately without the need to transform them to the GoTo.Rename type.

If in the future the Rename result types needs additional types, then the transformation will be applied and we can seperate them:

sealed trait Rename  extends GoTo
sealed trait GoToRef extends GoTo

If you prefer, I can seperate them now? I was just saving it from doing a transformation, which really isn't that expensive.

Copy link
Member

Choose a reason for hiding this comment

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

ok let's keep it like this for now then.

Copy link
Member

@tdroxler tdroxler left a comment

Choose a reason for hiding this comment

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

Minor comment, but LGTM

I tested it and it works great.

I have one remarks compared to metals, if I try to rename Future, I will directly get a notification: Nothing to rename.

While here if I try to rename Contract it will sill ask me with what I want to rename and when i confirm, nothing happen (as expected).

I just found in the lsp doc that there's a prepare rename request to test the validity of the request.

I'll open an issue with that prepare rename request, it improves a bit the UX.

@simerplaha
Copy link
Member Author

I just found in the lsp doc that there's a prepare rename request to test the validity of the request.

On this is useful. Thank you.

Base automatically changed from distinct_references to master October 16, 2024 07:45
@simerplaha
Copy link
Member Author

simerplaha commented Oct 17, 2024

Hey @tdroxler, will wait until #318 is resolved before merging this. Currently, renaming is only applied to the current member and not all occurrences. The issues details it.

@simerplaha simerplaha merged commit 10b9c53 into master Oct 29, 2024
3 checks passed
@simerplaha simerplaha deleted the rename branch October 29, 2024 00:07
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.

Implement rename request
2 participants