-
Notifications
You must be signed in to change notification settings - Fork 28
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
Enhance schema hover link to jump to the right table #212
base: main
Are you sure you want to change the base?
Conversation
I have signed the CLA! |
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.
Hi @faraazahmad! I was looking through the issues and #52 caught my attention. This looks like a great start to me! Although I'm not a maintainer, so that doesn't carry much weight.
I used part of your code and implemented the remaining steps to include the table location in the schema hover link. If you're interested, I opened a PR in my forked repository that is working for me locally. I didn't include any tests, and I'm new to sorbet so the type assertions are probably wrong.
Best regards!
node.block.body.child_nodes.each do |child_node| | ||
next unless child_node.is_a?(Prism::CallNode) | ||
next unless child_node.name == :create_table | ||
|
||
table_name = child_node.arguments.child_nodes.first.content | ||
@tables[table_name.classify] = child_node.location | ||
end |
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.
It is not necessary to traverse the nodes directly within a Prism::Visitor
. Per the documentation:
def visit_call_node(node)
if node.name == "foo"
# Do something with the node
end
# Call super so that the visitor continues walking the tree
super
end
So this could be restructured to look like:
def visit_call_node(node)
if node.name == 'create_table'
# ensure the first argument is a string node and store the location
end
super
end
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.
Hi @wbhouston! Thank you for these changes, they look great to me! Feel free to commit these changes to this PR
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.
@faraazahmad it won't be possible for @wbhouston to push to your branch.
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.
@wbhouston I tried your PR and it seems to work well. I do have some feedback, but I'd like if you can first work with @faraazahmad to get the work into a single PR against Shopify/ruby-lsp-rails
. I'll ensure you are both credited if it is merged.
Thanks @andyw8 ! I'll add those changes in my fork |
Hi @faraazahmad, I tried the latest version and it seems to not be jumping to the line in |
sig { void } | ||
def parse_schema | ||
parse_result = Prism::parse_file(schema_path) | ||
return unless parse_result.success? |
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.
I don't think this check if necessary. The schema.rb
file is generated, so normally it would always be valid Ruby. But in addition to that, Prism is error tolerant, so even if it cannot parse the entire file, it will still return a partial AST, which we can use to find tables.
module Rails | ||
class SchemaCollector < Prism::Visitor | ||
extend T::Sig | ||
extend T::Generic |
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.
This probably doesn't need to be generic.
project_root = T.let( | ||
Bundler.with_unbundled_env { Bundler.default_gemfile }.dirname, | ||
Pathname, | ||
) |
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 should be passing this as an argument from client
. Otherwise, we end up with the same information in two separate places, which is likely to get out of sync.
Co-authored-by: Vinicius Stock <[email protected]>
Hey thanks for the reviews! And sorry about the delay I've been busy with a couple of things I'll try and finish this soon |
@vinistock I've made the changes as needed, but I'm unable to figure out a Sorbet issue. Since, |
@faraazahmad I pushed a commit to your branch to address the typing and linting issues. I tried locally and it seems to be working well, but I'll let you confirm before merging. |
|
||
require "test_helper" | ||
|
||
SCHEMA_FILE = <<~RUBY |
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 move this into SchemaCollectorTest
so that it's not defined globally.
Let's also update the PR title to be more descriptive for when it appears in the release notes. |
Reminder for myself: Check how this performs for apps with a high number of tables, e.g. Core. |
super() | ||
|
||
@tables = T.let({}, T::Hash[String, Prism::Location]) | ||
@schema_path = T.let(project_root.join("db", "schema.rb").to_s, String) |
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.
This isn't quite right as some apps use structure.sql
instead.
If you search you'll see we use schema_dump_path
elsewhere to get the path.
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.
This means this collector will need to communicate with the server. The path for the schema is unlikely to change, so maybe there's room for a refactor where we fetch the schema path once and reuse it here and in hover.
Maybe we can move forward with this and simply not populate anything if db/schema.rb
doesn't exist? That way we can provide the feature for schema users and enhance it later.
super() | ||
|
||
@tables = T.let({}, T::Hash[String, Prism::Location]) | ||
@schema_path = T.let(project_root.join("db", "schema.rb").to_s, String) |
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.
This means this collector will need to communicate with the server. The path for the schema is unlikely to change, so maybe there's room for a refactor where we fetch the schema path once and reuse it here and in hover.
Maybe we can move forward with this and simply not populate anything if db/schema.rb
doesn't exist? That way we can provide the feature for schema users and enhance it later.
Co-authored-by: Vinicius Stock <[email protected]>
Add a
SchemaCollector
class that expectsschema.rb
AST and stores the location for all the defined tables.For issue #52