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

Add rename support for constants #2626

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Add rename support for constants #2626

merged 1 commit into from
Oct 2, 2024

Conversation

vinistock
Copy link
Member

Motivation

Closes #57

This PR adds rename support for constants, modules and classes.

Implementation

  1. Created a ReferenceFinder. This is essentially a simplified copy of our declaration listener. I believe that either the declaration listener can be modified to support collecting references or we can extract some abstraction for both to use. However, I would like to start with a copy, so that we can see the patterns emerge as we add support for more renames
  2. Then I added the rename request

Automated Tests

Added tests.

@vinistock vinistock added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Sep 25, 2024
@vinistock vinistock self-assigned this Sep 25, 2024
@vinistock vinistock requested a review from a team as a code owner September 25, 2024 19:24
@vinistock vinistock requested review from andyw8 and st0012 September 25, 2024 19:24
@vinistock

This comment was marked as resolved.

Copy link

netlify bot commented Oct 1, 2024

Deploy Preview for ruby-lsp failed.

Name Link
🔨 Latest commit a40caf8
🔍 Latest deploy log https://app.netlify.com/sites/ruby-lsp/deploys/66fd521a2a97c00008cf085a

@vinistock vinistock force-pushed the vs-add-rename-support branch from ae8a87a to a40caf8 Compare October 2, 2024 14:00
@vinistock vinistock requested a review from st0012 October 2, 2024 14:01
@andyw8
Copy link
Contributor

andyw8 commented Oct 2, 2024

Investigating a hang with previews:

Screenshot 2024-10-02 at 1 10 33 PM

lib/ruby_lsp/requests/rename.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/rename.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/rename.rb Show resolved Hide resolved
@andyw8
Copy link
Contributor

andyw8 commented Oct 2, 2024

Should we allow people to change namespaces with this? e.g. renaming Foo to Bar::Foo

It 'works' but does complicate some things.

jekyll/index.markdown Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb Outdated Show resolved Hide resolved
jekyll/index.markdown Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs-add-rename-support branch from a40caf8 to f0cbd0d Compare October 2, 2024 18:36
@vinistock vinistock requested review from st0012 and andyw8 October 2, 2024 18:39
@vinistock vinistock force-pushed the vs-add-rename-support branch from f0cbd0d to c60e050 Compare October 2, 2024 19:03
@andyw8
Copy link
Contributor

andyw8 commented Oct 2, 2024

🤔 If a constant is already defined, should we prevent someone to renaming to that constant?

@vinistock vinistock force-pushed the vs-add-rename-support branch from c60e050 to 21f56af Compare October 2, 2024 19:24
@vinistock
Copy link
Member Author

🤔 If a constant is already defined, should we prevent someone to renaming to that constant?

Added a check for this. Great catch.

@vinistock vinistock requested a review from andyw8 October 2, 2024 19:24
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I still prefer assigning attributes to ivars. But it's a small detail that can be discussed later.

@vinistock vinistock force-pushed the vs-add-rename-support branch from 21f56af to 377e988 Compare October 2, 2024 20:18
@vinistock vinistock enabled auto-merge (squash) October 2, 2024 20:27
@vinistock vinistock merged commit 9c1f570 into main Oct 2, 2024
37 checks passed
@vinistock vinistock deleted the vs-add-rename-support branch October 2, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 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.

Add rename support to the ruby-lsp
3 participants