Index includes, prepends, and extends with RBS indexer#617
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
soutaro
left a comment
soutaro
left a comment
There was a problem hiding this comment.
Looks good to me.
Note: Rubydex doesn't index the comments for mixins.
| include Bar::Baz | ||
| end | ||
| " | ||
| }); |
There was a problem hiding this comment.
The test name says "qualified name" but assert_def_mixins_eq\! only checks the leaf segment ("Baz"), not the full Bar::Baz path. I wonder if we should assert the full path here to confirm the qualified name resolves correctly — maybe using assert_name_path_eq\! on the mixin's reference?
There was a problem hiding this comment.
Yeah, we should assert on the entire string Bar::Baz and not just Baz. Although, that's an existing limitation. If this involves updating too many tests, let's do it in a different PR.
There was a problem hiding this comment.
I'll take the suggestion and fix the macro to assert the full qualified path in a follow-up PR 👍
| @@ -486,6 +537,62 @@ mod tests { | |||
| }); | |||
There was a problem hiding this comment.
The Ruby indexer has index_includes_at_top_level to confirm top-level mixins are silently ignored. Should we add an equivalent here for the RBS indexer to document that behavior explicitly?
There was a problem hiding this comment.
Wait, is that the case? If so, we have a bug. Mixin operations at the top level impact Object:
module Foo; end
include Foo
puts Object.ancestors.inspect
# => [Object, Foo, Kernel, BasicObject]Regardless, we should add the test.
There was a problem hiding this comment.
Including at the top level returns a parse error.
In RBS docs, mixins are not part of the declarations and I believe we're hitting the default case here.
Doesn't look like this is an issue for the RBS indexer
| @@ -486,6 +537,62 @@ mod tests { | |||
| }); | |||
There was a problem hiding this comment.
Wait, is that the case? If so, we have a bug. Mixin operations at the top level impact Object:
module Foo; end
include Foo
puts Object.ancestors.inspect
# => [Object, Foo, Kernel, BasicObject]Regardless, we should add the test.
| include Bar::Baz | ||
| end | ||
| " | ||
| }); |
There was a problem hiding this comment.
Yeah, we should assert on the entire string Bar::Baz and not just Baz. Although, that's an existing limitation. If this involves updating too many tests, let's do it in a different PR.
alexcrocha
force-pushed
the
02-27-index_includes_prepends_and_extends_with_rbs_indexer
branch
from
a305dc9 to
fab7552
Compare
alexcrocha
force-pushed
the
02-27-index_globals_with_rbs_indexer
branch
from
f77fbfe to
f7e2542
Compare
This adds visit_include_node, visit_prepend_node, and visit_extend_node with a shared index_mixin helper. Mixins resolve into the ancestor chain matching the Ruby indexer. Also moves assert_def_mixins_eq! to the shared test macros
alexcrocha
force-pushed
the
02-27-index_includes_prepends_and_extends_with_rbs_indexer
branch
from
fab7552 to
19add0c
Compare
alexcrocha
deleted the
02-27-index_includes_prepends_and_extends_with_rbs_indexer
branch
4 participants
One more step towards #87.
This PR adds
visit_include_node,visit_prepend_node, andvisit_extend_nodewith a sharedindex_mixinhelper. Mixins resolve into the ancestor chain matching the Ruby indexer.Also moves
assert_def_mixins_eq!to the shared test macros inlocal_graph_test.