-
Notifications
You must be signed in to change notification settings - Fork 37
Add documentSymbol support to schema.rb #660
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
base: main
Are you sure you want to change the base?
Add documentSymbol support to schema.rb #660
Conversation
vinistock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just change the way that the stack related things are being handled. Other than that, the change looks good
|
|
||
| return unless node.parent.name == :ActiveRecord && node.name == :Schema | ||
|
|
||
| @namespace_stack << node.full_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't push this to the namespace stack. The goal of the stack is to remember the module/class nesting that we're currently inside.
Originally, we needed to return early if the stack was empty, but this addition changes that premise.
Let's do the following:
def on_call_node_enter(node)
message = node.message
return unless message
# Handle the tables here
handle_schema_table(node)
receiver = node.receiver
return if receiver && !receiver.is_a?(Prism::SelfNode)
# Handle cases that require being inside a certain level of nesting
return if @namespace_stack.empty?
case message
when *Support::Callbacks::ALL, "validate"
handle_all_arg_types(node, message)
when "validates", "validates!", "validates_each", "belongs_to", "has_one", "has_many", "has_and_belongs_to_many", "attr_readonly", "scope"
handle_symbol_and_string_arg_types(node, message)
when "validates_with"
handle_class_arg_types(node, message)
when "create_table"
handle_create_table(node)
else
content = extract_test_case_name(node)
if content
append_document_symbol(
name: content,
selection_range: range_from_node(node),
range: range_from_node(node),
)
end
end
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @vinistock, thanks for the feedback. I'm almost done incorporating it.
I just have one quick question about this, don't we need to check that we are inside the ActiveRecord::Schema#define block before adding the symbols? If this is correct I'm not sure how to do that without using the namespace stack.
Otherwise calling create_table from anywhere will append to the symbols which doesn't seem right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good point. We can definitely check for the ActiveRecord::Schema#define call, but let's simply remember this state separately in another instance variable.
For example:
def initialize(...)
# ...
@inside_schema = false #: bool
end
def on_call_node_enter(node)
# ...
@inside_schema = true if message == "define" && constant_name(receiver) == "ActiveRecord::Schema"
end
def on_call_node_leave(node)
# ...
@inside_schema = false if message == "define" && constant_name(receiver) == "ActiveRecord::Schema"
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinistock this should be good to go. I added a test case to guard us against the create_table method adding symbols outside the ActiveRecord::Schema#define block. Please let me know of any other feedback.
close #361