Add workspace symbols #106

faustinoaq · 2018-05-05T18:51:36Z

Fixes #102 🎉

bew · 2018-05-05T18:57:30Z

src/scry/symbol.cr

+
+  class WorkspaceSymbolProcessor
+    def initialize(@msg_id : Int32 | String, @root_path : String, @query : String)
+      @all_files = Dir.glob("#{root_path}/**/*.cr")


Why doesn't this use the workspace, and find files based on prelude and requires?

and find files based on prelude and requires?

What do you mean? 😅 This will get all "workspace" files to analyze all "workspace" symbols

Oh ok, so this symbol feature is just to search in your project (your own code), not everywhere (stdlib, other shards, etc) ?

Yep, just your code 😅

Although this should work on shards as well since lib is analyzed too.

Ok it's fine then. I think I'd even restrict to src, to not show implementation detail symbols from shards

@bew Well, I guess I like your idea of listing stdlib symbols, so, this feature will support listing symbols from:

Your code

Your shards

Crystal stdlib

And works very fast!!! 🎉 and stdlib is memoized ✨

bew · 2018-05-05T19:01:00Z

spec/scry/symbol_spec.cr

+      it "return Symbols list with query match" do
+        processor = WorkspaceSymbolProcessor.new(0, "#{ROOT_PATH}/src", "salut")
+        response = processor.run
+        result = response.result.as(Array(SymbolInformation)).try(&.first)


try is uselesw here as you can the object to a non-nilable type juste before.

bew · 2018-05-05T19:01:59Z

spec/scry/symbol_spec.cr

+      end
+
+      it "return Symbols list with query match" do
+        processor = WorkspaceSymbolProcessor.new(0, "#{ROOT_PATH}/src", "salut")


Use regex here? Or add another spec for a regex query?

A little Use File.join commit ? 😃

bew · 2018-05-05T22:28:16Z

src/scry/symbol.cr

+          visitor = SymbolVisitor.new("file://#{file}")
+          parser = Crystal::Parser.new(File.read(file))
+          parser.filename = file
+          node = parser.parse


what if there is a syntax error in one of the file? I think the exception needs to be rescued and the file skipped.

Is it possible to send errors in addition to valid results? (say, here are the results for files X & Y, but there is an error with file Z)

I guess I can handle the error, but the response should still be managed by diagnostic provider

I think I can return an empty array if syntax error is found

Looking at https://microsoft.github.io/language-server-protocol/specification#workspace-symbols-request-leftwards_arrow_with_hook you should be able to return null and an error in case an exception happens during the workspace symbol request.

I think it could be possible to return a result & an error? but how the valid_result/error is handled would vastly depend on the language client implementation..

Well, Actually we're managing that via response_error.cr, although I think for better user experience we should ignore parsing errors here since the diagnostics already do this work.

I don't want to see a popup saying "LSP server has an error" every time I try to list symbols on files with syntax errors 😅

faustinoaq · 2018-05-05T23:26:07Z

stdlib symbols has been added as well 🎉

bew · 2018-05-05T23:54:10Z

src/scry/symbol.cr

+      @workspace_files = Dir.glob(File.join(root_path, "**", "*.cr"))
+      if @@crystal_path_files.empty?
+        # Memoize crystal stdlib
+        @@crystal_path_files = search_symbols(Dir.glob(File.join(Scry.default_crystal_path, "**", "*.cr")), ".*")


rename to crystal_path_symbols as it stores symbols

bew · 2018-05-05T23:56:36Z

src/scry/symbol.cr

-          symbols.concat visitor.symbols.select(&.name.match(Regex.new(@query)))
-        end
+        symbols.concat search_symbols(@workspace_files, @query)
+        symbols.concat @@crystal_path_files.select(&.name.match(Regex.new(@query)))


You could store the Regex object in @query in the initialize, instead of storing the String and re-create/re-compile the regex on each usage.

bew · 2018-05-06T00:11:08Z

Also, why in you last screenshot some entries have non-absolute path even though it's not in the current workspace (e.g: pow_f64 or FileModule) ?

faustinoaq · 2018-05-06T00:16:49Z

@bew Interesting I didn't realize of that 👍

I think SymbolInformation is too long and VSCode just hide it 😅

Although, If you test it, still works

bew · 2018-05-06T00:24:46Z

src/scry/symbol.cr

      end
      ResponseMessage.new(@msg_id, symbols)
    end

-    def search_symbols(files, query)
+    def search_symbols(files, query_regex)


Not sure if it's a good idea, but you could have search_symbols as a class method (as it doesn't depend on any ivars), and use special getter for @@crystal_path_symbols:

@@crystal_path_symbols : Array(SymbolInformation)? def self.crystal_path_symbols @@crystal_path_symbols ||= begin crystal_path_files = Dir.glob(File.join(Scry.default_crystal_path, "**", "*.cr")) search_symbols(crystal_path_files, Regex.new(".*")) end end # and when you need crystal_path_symbols, you just call the getter, which will return # `@@crystal_path_symbols` if it's not nil, and will search the symbols if it's nil. def self.search_symbols(x, y) end

This way you can remove the cvar init from the initiailizer and keep it clean 😃

bew · 2018-05-06T05:06:29Z

src/scry/symbol.cr

      @query_regex = Regex.new(@query)
    end

    def run
      symbols = [] of SymbolInformation
      unless @query.empty?
-        symbols.concat search_symbols(@workspace_files, @query_regex)
-        symbols.concat @@crystal_path_symbols.select(&.name.match(@query_regex))
+        symbols.concat self.class.search_symbols(@workspace_files, @query_regex)


Weird I thought it was simpler to call..

Another way is WorkspaceSymbolProcessor.search_symbols or {{@type}}.search_symbols (as you like, self.class is fine too)

I think I prefer self.class 😅

bmulvihill · 2018-05-06T14:17:37Z

src/scry/symbol.cr

+    def self.search_symbols(files, query_regex)
+      symbols = [] of SymbolInformation
+      files.each do |file|
+        visitor = SymbolVisitor.new("file://#{file}")


I wonder if there is anyway we can reuse the SymbolProcessor here since the code is so similar? Although it would probably need to be decoupled from the ResponseMessage, maybe we can refactor that later.

faustinoaq added 2 commits May 5, 2018 13:50

Add support for workspace symbols

25d171f

Add spec for workspace symbols

3c6f673

faustinoaq requested review from bmulvihill, laginha87 and a team May 5, 2018 18:53

faustinoaq added enhancement review required labels May 5, 2018

bew reviewed May 5, 2018

View reviewed changes

faustinoaq added 2 commits May 5, 2018 15:59

Remove try and add regex query spec for workspace symbols

d46052b

Fix spec

10830b1

bew reviewed May 5, 2018

View reviewed changes

faustinoaq added 3 commits May 5, 2018 18:11

Add missing RequestType

c3ba087

Add stdlib (momoized) to workspace symbols

93704f5

Add specs for stdlib symbols

0675f49

Fix specs

81ccecf

bew reviewed May 5, 2018

View reviewed changes

Rename crystal_path_files to crystal_path_symbols

df9bc43

Store query regex on initializer

a0db775

bew reviewed May 6, 2018

View reviewed changes

Use class variables and class methods

719c354

bew reviewed May 6, 2018

View reviewed changes

faustinoaq requested a review from bew May 6, 2018 06:41

bew approved these changes May 6, 2018

View reviewed changes

Use full ROOT_PATH on spec to rest invalid files

c2543fd

bmulvihill reviewed May 6, 2018

View reviewed changes

bmulvihill approved these changes May 6, 2018

View reviewed changes

faustinoaq merged commit 81a7171 into master May 6, 2018

faustinoaq deleted the fa/workspace-symbols branch May 6, 2018 19:41

faustinoaq removed the review required label May 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workspace symbols #106

Add workspace symbols #106

faustinoaq commented May 5, 2018

bew May 5, 2018

faustinoaq May 5, 2018

bew May 5, 2018 •

edited

Loading

faustinoaq May 5, 2018

bew May 5, 2018

faustinoaq May 5, 2018

bew May 5, 2018

faustinoaq May 5, 2018

bew May 5, 2018

faustinoaq May 5, 2018

bew May 6, 2018 •

edited

Loading

bew May 5, 2018

faustinoaq May 5, 2018

bew May 5, 2018 •

edited

Loading

faustinoaq May 5, 2018 •

edited

Loading

faustinoaq commented May 5, 2018

bew May 5, 2018

faustinoaq May 6, 2018

bew May 5, 2018

faustinoaq May 6, 2018

bew commented May 6, 2018

faustinoaq commented May 6, 2018 •

edited

Loading

bew May 6, 2018 •

edited

Loading

faustinoaq May 6, 2018

bew May 6, 2018

faustinoaq May 6, 2018

bew May 6, 2018

bmulvihill May 6, 2018

Add workspace symbols #106

Add workspace symbols #106

Conversation

faustinoaq commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew May 5, 2018 • edited Loading

Choose a reason for hiding this comment

faustinoaq May 5, 2018 • edited Loading

Choose a reason for hiding this comment

faustinoaq commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew commented May 6, 2018

faustinoaq commented May 6, 2018 • edited Loading

bew May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew May 5, 2018 •

edited

Loading

bew May 6, 2018 •

edited

Loading

bew May 5, 2018 •

edited

Loading

faustinoaq May 5, 2018 •

edited

Loading

faustinoaq commented May 6, 2018 •

edited

Loading

bew May 6, 2018 •

edited

Loading