-
Notifications
You must be signed in to change notification settings - Fork 183
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
Search for class variables completion candidates in attached namespace #3100
Search for class variables completion candidates in attached namespace #3100
Conversation
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
6edf374
to
e598ccb
Compare
I'd like to propose extracting the repeated conditions into a helper: diff --git a/lib/ruby_indexer/lib/ruby_indexer/index.rb b/lib/ruby_indexer/lib/ruby_indexer/index.rb
index 4dbf7a3c..cf206f4d 100644
--- a/lib/ruby_indexer/lib/ruby_indexer/index.rb
+++ b/lib/ruby_indexer/lib/ruby_indexer/index.rb
@@ -593,13 +593,7 @@ module RubyIndexer
entries = self[variable_name]&.grep(Entry::ClassVariable)
return unless entries&.any?
- name_parts = owner_name.split("::")
- ancestors = if name_parts.last&.start_with?("<Class:")
- attached_name = T.must(name_parts[0..-2]).join("::")
- linearized_ancestors_of(attached_name)
- else
- linearized_ancestors_of(owner_name)
- end
+ ancestors = linearize_ancestors_for_class_variable(owner_name)
return if ancestors.empty?
entries.select { |e| ancestors.include?(e.owner&.name) }
@@ -623,15 +617,8 @@ module RubyIndexer
# Class variables are only owned by the attached class in our representation. If the owner is in a singleton
# context, we have to search for ancestors of the attached class
if class_variables.any?
- name_parts = owner_name.split("::")
-
- if name_parts.last&.start_with?("<Class:")
- attached_name = T.must(name_parts[0..-2]).join("::")
- attached_ancestors = linearized_ancestors_of(attached_name)
- variables.concat(class_variables.select { |e| attached_ancestors.any?(e.owner&.name) })
- else
- variables.concat(class_variables.select { |e| ancestors.any?(e.owner&.name) })
- end
+ ancestors = linearize_ancestors_for_class_variable(owner_name)
+ variables.concat(class_variables.select { |e| ancestors.any?(e.owner&.name) })
end
variables.uniq!(&:name)
@@ -644,17 +631,7 @@ module RubyIndexer
# Avoid wasting time linearizing ancestors if we didn't find anything
return entries if entries.empty?
- name_parts = owner_name.split("::")
-
- # Class variable are always stored in association with the attached class. Even if we're searching in a singleton
- # namespace, we have to do the lookup in the attached ancestors
- ancestors = if name_parts.last&.start_with?("<Class:")
- attached_name = T.must(name_parts[0..-2]).join("::")
- linearized_ancestors_of(attached_name)
- else
- linearized_ancestors_of(owner_name)
- end
-
+ ancestors = linearize_ancestors_for_class_variable(owner_name)
variables = entries.select { |e| ancestors.any?(e.owner&.name) }
variables.uniq!(&:name)
variables
@@ -756,6 +733,19 @@ module RubyIndexer
private
+ # Class variable are always stored in association with the attached class. Even if we're searching in a singleton
+ # namespace, we have to do the lookup in the attached ancestors
+ sig { params(name: String).returns(T::Array[String]) }
+ def linearize_ancestors_for_class_variable(name)
+ name_parts = name.split("::")
+ if name_parts.last&.start_with?("<Class:")
+ attached_name = T.must(name_parts[0..-2]).join("::")
+ linearized_ancestors_of(attached_name)
+ else
+ linearized_ancestors_of(name)
+ end
+ end
+
# Runs the registered included hooks
sig { params(fully_qualified_name: String, nesting: T::Array[String]).void }
def run_included_hooks(fully_qualified_name, nesting) |
e598ccb
to
9f9109f
Compare
9f9109f
to
f2bb991
Compare
Merge activity
|
f2bb991
to
8b21a19
Compare
#3100) ### Motivation Class variables can be referenced both from singleton and attached contexts. To save memory, we decided to always associate class variables with only the attached version of namespaces, but we didn't account for this in either resolving class variables or finding completion candidates. ### Implementation We need to always search for class variables within the attached namespace's ancestor chain. ### Automated Tests Added tests that reproduce the bugs.
8b21a19
to
41df177
Compare
Motivation
Class variables can be referenced both from singleton and attached contexts. To save memory, we decided to always associate class variables with only the attached version of namespaces, but we didn't account for this in either resolving class variables or finding completion candidates.
Implementation
We need to always search for class variables within the attached namespace's ancestor chain.
Automated Tests
Added tests that reproduce the bugs.