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.

Sign up for GitHub

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

Jump to bottom

Use RBS to index methods #2157

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Use RBS to index methods #2157

merged 1 commit into from
Jun 12, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jun 11, 2024 •
edited
Loading

This PR continues the RBS indexing work to include singleton and instance methods from modules and classes.

@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch from 284fdb9 to 8655952 Compare June 11, 2024 16:46
@andyw8 andyw8 changed the base branch from main to implement-#2104 June 11, 2024 16:47
@st0012 st0012 force-pushed the implement-#2104 branch 4 times, most recently from 95acdb1 to 902f098 Compare June 11, 2024 17:07
@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch 2 times, most recently from 9667733 to 3a2c40f Compare June 11, 2024 17:15
Base automatically changed from implement-#2104 to main June 11, 2024 18:28
@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch 2 times, most recently from 2d207ac to 2d867c5 Compare June 11, 2024 18:36
@andyw8 andyw8 mentioned this pull request Jun 11, 2024
Closed
@andyw8 andyw8 added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Jun 11, 2024
@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch 4 times, most recently from a49cde6 to b253e57 Compare June 11, 2024 19:19
andyw8
andyw8 commented Jun 11, 2024
View reviewed changes
lib/ruby_indexer/test/index_test.rb Outdated
@@ -92,6 +92,7 @@ class Something
end

def test_fuzzy_search
@index = Index.new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same approach as @st0012 took in #2148 but we should really better separate the different indexing behaviours.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to separate different indexing behaviours because then we'll be deviating from what these features are actually going to be used. It'll also make tests harder to maintain in the long term.

Now that we also have method names in the PR, we should use this opportunity to decide a few constant/method names that can better avoid false-positive in such tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test file is already over 1000 lines long so I'm wary of adding more to it. How about this approach:

  • one test file for the Ruby indexing (with Prism)
  • one test file for the RBS indexing
  • one test file which verifies how they work in combination.

@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch from b253e57 to 87ce179 Compare June 11, 2024 19:49
andyw8
andyw8 commented Jun 11, 2024
View reviewed changes
lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb
file_path = member.location.buffer.name
location = to_ruby_indexer_location(member.location)
comments = Array(member.comment&.string)
parameters_node = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be in a follow-up.

@andyw8
Copy link
Contributor Author

andyw8 commented Jun 11, 2024

@vinistock @st0012 still WIP but any early input is welcome.

st0012
st0012 reviewed Jun 11, 2024
View reviewed changes
lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

owner = class_entry

klass = member.instance? ? Entry::InstanceMethod : Entry::SingletonMethod
Copy link
Member

Choose a reason for hiding this comment

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

Headsup that these classes will be merged after #2142

lib/ruby_indexer/test/index_test.rb Outdated
@@ -92,6 +92,7 @@ class Something
end

def test_fuzzy_search
@index = Index.new
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to separate different indexing behaviours because then we'll be deviating from what these features are actually going to be used. It'll also make tests harder to maintain in the long term.

Now that we also have method names in the PR, we should use this opportunity to decide a few constant/method names that can better avoid false-positive in such tests.

@andyw8
Copy link
Contributor Author

andyw8 commented Jun 12, 2024

@st0012 I worked with @vinistock to adjust some tests (had to pick some weird names!).

@andyw8 andyw8 changed the title WIP: Use RBS to index methods Use RBS to index methods Jun 12, 2024
@andyw8 andyw8 marked this pull request as ready for review June 12, 2024 16:32
@andyw8 andyw8 requested a review from a team as a code owner June 12, 2024 16:32
@andyw8 andyw8 requested review from st0012 and vinistock June 12, 2024 16:32
@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch from 0e78d0f to 97970e8 Compare June 12, 2024 16:32
vinistock
vinistock approved these changes Jun 12, 2024
View reviewed changes
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

After the parameters follow up, let's ensure we're also handling attributes

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb Outdated Show resolved Hide resolved
st0012
st0012 approved these changes Jun 12, 2024
View reviewed changes
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.

Not a blocker as it's probably an edge case anyway: Is it necessary to call handle_member in process_declaration as well?

@andyw8 andyw8 force-pushed the andyw8/use-rbs-to-index-methods branch from 97970e8 to 8bb49e7 Compare June 12, 2024 17:52
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 12, 2024

@st0012 do you mean in case of methods which are defined outside of any module/class?

@andyw8 andyw8 enabled auto-merge (squash) June 12, 2024 17:55
@st0012
Copy link
Member

st0012 commented Jun 12, 2024

do you mean in case of methods which are defined outside of any module/class?
Exactly. I'm not sure if there's any but it's probably worth checking and documenting the result?

@andyw8
Copy link
Contributor Author

andyw8 commented Jun 12, 2024

The typecheck fails:

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb:39: This condition was always falsy (FalseClass)
    39 |      when RBS::AST::Members::MethodDefinition

@andyw8 andyw8 added server This pull request should be included in the server gem's release notes and removed server This pull request should be included in the server gem's release notes labels Jun 12, 2024
@andyw8 andyw8 merged commit 28a7ebe into main Jun 12, 2024
34 checks passed
@andyw8 andyw8 deleted the andyw8/use-rbs-to-index-methods branch June 12, 2024 18:36
st0012 added a commit that referenced this pull request Jun 12, 2024
@st0012
Resolve missed conflicts
25462ea 
PR #2157 relies on Entry::InstanceMethod and Entry::SingletonMethod,
which were merged together in #2142. But because the PRs are merged
side-by-side without rebase, this resulted in a broken build.

This commit reflects the class changes in the RBSIndexer.
@st0012 st0012 mentioned this pull request Jun 12, 2024
Merged
st0012 added a commit that referenced this pull request Jun 12, 2024
@st0012
Resolve missed conflicts (#2177)
d706167 
PR #2157 relies on Entry::InstanceMethod and Entry::SingletonMethod,
which were merged together in #2142. But because the PRs are merged
side-by-side without rebase, this resulted in a broken build.

This commit reflects the class changes in the RBSIndexer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

@vinistock vinistock vinistock approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyw8 @st0012 @vinistock