-
Notifications
You must be signed in to change notification settings - Fork 16
Move DOT visualization to the Ruby CLI #966
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ USAGE = <<~TEXT | |
| Use `query --schema` to describe the queryable schema (labels, | ||
| relationships, properties) without indexing the workspace. | ||
| console Open an interactive session with a populated graph for the current workspace | ||
| dot [PATH] Output a Graphviz DOT visualization (workspace defaults to the current dir) | ||
| mcp [PATH] Run the MCP server for AI assistants (workspace defaults to the current dir) | ||
| help Show this help message | ||
|
|
||
|
|
@@ -55,8 +56,8 @@ def with_timer(io, message) | |
| end | ||
|
|
||
| # Builds the workspace graph, sending progress messages to `progress_io`. | ||
| def build_graph(progress_io) | ||
| graph = Rubydex::Graph.new | ||
| def build_graph(progress_io, workspace_path) | ||
| graph = Rubydex::Graph.new(workspace_path: File.expand_path(workspace_path)) | ||
| graph.load_config | ||
| with_timer(progress_io, "Indexing workspace...") { graph.index_workspace } | ||
| with_timer(progress_io, "Resolving graph...") { graph.resolve } | ||
|
|
@@ -68,6 +69,8 @@ require "rubydex" | |
| # Resolve the command into an operation on a populated graph. Each command parses its own options | ||
| # and does any graph-independent work here; a command that needs no graph (like `query --schema`) | ||
| # handles itself and exits. Whatever falls through returns a lambda that runs against the graph. | ||
| workspace_path = Dir.pwd | ||
|
|
||
| operation = | ||
| case command | ||
| when "query" | ||
|
|
@@ -106,6 +109,28 @@ operation = | |
| rescue ArgumentError => e | ||
| abort(e.message) | ||
| end | ||
| when "dot" | ||
| show_builtins = false | ||
| parser = OptionParser.new do |p| | ||
| p.banner = "Usage: rdx dot [PATH] [options]" | ||
| p.on("--show-builtins", "Include built-in declarations in DOT output") { show_builtins = true } | ||
| p.on("-h", "--help", "Show this help") do | ||
| puts p | ||
| exit | ||
| end | ||
| end | ||
| begin | ||
| parser.parse! | ||
| rescue OptionParser::ParseError => e | ||
| abort_with_usage(e.message) | ||
| end | ||
|
|
||
| workspace_path = ARGV.shift || Dir.pwd | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: passing an explicit argument here won't include that workspace's dependencies without setting up the Bundle based on its Gemfile, so this has difference behaviour than pwd. |
||
| abort_with_usage("unexpected argument: #{ARGV.first}") unless ARGV.empty? | ||
|
|
||
| lambda do |graph| | ||
| print(graph.to_dot(show_builtins: show_builtins)) | ||
| end | ||
| when "console" | ||
| OptionParser.new do |parser| | ||
| parser.banner = "Usage: rdx console" | ||
|
|
@@ -151,5 +176,5 @@ operation = | |
|
|
||
| # Everything that reaches here operates on a populated graph. Progress goes to stderr so stdout | ||
| # carries only the command's output (e.g. for piping a query's JSON). | ||
| graph = build_graph($stderr) | ||
| graph = build_graph($stderr, workspace_path) | ||
| operation.call(graph) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "open3" | ||
| require "rbconfig" | ||
|
|
||
| module Test | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing behaviour through executables feels weird. I think this is a result of us not having a proper CLI object that can be unit tested yet, which forces us to reach into actually shelling out. Since we already need to refactor the CLI for the linter, can we avoid adding this? We can add the dot scenarios as unit tests for the CLI as soon as we add it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be streamlined on latest |
||
| module Helpers | ||
| module WithExecutable | ||
| private | ||
|
|
||
| #: (*String) -> [String, String, Process::Status] | ||
| def run_executable(*arguments) | ||
| Open3.capture3( | ||
| RbConfig.ruby, | ||
| "-rbundler/setup", | ||
| executable_path, | ||
| *arguments, | ||
| ) | ||
| end | ||
|
|
||
| #: -> String | ||
| def executable_path | ||
| File.expand_path("../../exe/rdx", __dir__) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "test_helper" | ||
| require "helpers/context" | ||
| require "helpers/executable" | ||
|
|
||
| class DotCLIIntegrationTest < Minitest::Test | ||
| include Test::Helpers::WithContext | ||
| include Test::Helpers::WithExecutable | ||
|
|
||
| def test_executable_outputs_graphviz | ||
| with_context do |context| | ||
| context.write!("simple.rb", "class SimpleClass; end") | ||
|
|
||
| stdout, stderr, status = run_executable("dot", context.absolute_path) | ||
|
|
||
| assert_predicate(status, :success?, stderr) | ||
| assert_includes(stderr, "Indexing workspace") | ||
| assert_includes(stderr, "Resolving graph") | ||
| assert_includes(stdout, "digraph rubydex") | ||
| assert_includes(stdout, "SimpleClass") | ||
| refute_includes(stdout, "rubydex:built-in") | ||
|
|
||
| stdout, stderr, status = run_executable("dot", "--show-builtins", context.absolute_path) | ||
|
|
||
| assert_predicate(status, :success?, stderr) | ||
| assert_includes(stdout, "rubydex:built-in") | ||
| end | ||
| end | ||
| 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.
If we're really going to allow the path argument, we should be explicit that it cannot include dependencies.
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 also find it interesting that we are indexing a workspace using the gemfile of some other workspace. Do we really need the workspace option?