Skip to content

Add rename support for constants#2626

Merged
vinistock merged 1 commit into
mainfrom
vs-add-rename-support
Oct 2, 2024
Merged

Add rename support for constants#2626
vinistock merged 1 commit into
mainfrom
vs-add-rename-support

Conversation

@vinistock
Copy link
Copy Markdown
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.

@netlify
Copy link
Copy Markdown

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

Comment thread lib/ruby_indexer/test/reference_finder_test.rb
@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
Copy Markdown
Contributor

andyw8 commented Oct 2, 2024

Investigating a hang with previews:

Screenshot 2024-10-02 at 1 10 33 PM

Comment thread lib/ruby_lsp/requests/rename.rb Outdated
Comment thread lib/ruby_lsp/requests/rename.rb Outdated
Comment thread lib/ruby_lsp/requests/rename.rb
@andyw8
Copy link
Copy Markdown
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.

Comment thread jekyll/index.markdown Outdated
Comment thread lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb Outdated
Comment thread lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb
Comment thread jekyll/index.markdown Outdated
Comment thread lib/ruby_indexer/lib/ruby_indexer/reference_finder.rb Outdated
@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 andyw8 and st0012 October 2, 2024 18:39
Comment thread test/requests/rename_test.rb
@vinistock vinistock force-pushed the vs-add-rename-support branch from f0cbd0d to c60e050 Compare October 2, 2024 19:03
@andyw8
Copy link
Copy Markdown
Contributor

andyw8 commented Oct 2, 2024

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

Comment thread lib/ruby_lsp/global_state.rb
@vinistock vinistock force-pushed the vs-add-rename-support branch from c60e050 to 21f56af Compare October 2, 2024 19:24
@vinistock
Copy link
Copy Markdown
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
Copy Markdown
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
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