Add class and module name completion #100

laginha87 · 2018-05-04T20:33:03Z

Hi all
This adds autocomplete for class names and module names.
It doesn't show any documentation for it yet.
Will add a gif to demo.

faustinoaq · 2018-05-04T23:53:46Z

Works like a charm! 🎉

laginha87 · 2018-05-05T07:20:49Z

Thanks for posting a gif example 👍

faustinoaq

works!!! 🥇

bew · 2018-05-05T16:35:43Z

spec/scry/completion_provider_spec.cr

+      it_completes("A", %w(Array Atomic ArgumentError AtExitHandlers))
+      it_completes("JSON::P", %w(ParseException Parser PullParser))
+      it_completes("JSON::Pa", %w(ParseException Parser))
+      it_completes("JSO", %w(JSON JSON::Any JSON::Builder JSON::Error JSON::Lexer JSON::Lexer::IOBased JSON::Lexer::StringBased JSON::ParseException JSON::Parser JSON::PullParser JSON::Token))


Can you add a spec for completion after JSON:: ?
And after :: ? (if it works)

bew · 2018-05-05T16:37:45Z

src/scry/completion/method_db.cr

@@ -20,6 +20,10 @@ module Scry::Completion
      end.select(&.name.starts_with? text).to_a
    end

+    def type_match(text : String) : Array(String)
+      @db.keys.select(&.starts_with?(text)).reject(&.ends_with?(".class"))


select!
reject!

@bew Why? are mutable methods faster?

Yeah I think so, they don't allocate a new array each time.

bew · 2018-05-05T16:40:36Z

src/scry/completion/method_db.cr

@@ -53,39 +57,54 @@ module Scry::Completion

  class Visitor < Crystal::Visitor
    property classes
+    property class_name = ""


I think it's better to use nil to denote the absence of class_name, so the compiler can help you instead of doing empty checks.

I think it's easier with an empty string cause we only interpolate stuff, otherwise I'll have to use not_nil! calls

I don't agree, I think it's always better to use the typing system for this kind of case.

For example in visit(node : Crystal::Def) where you use it, I'd use:

class_name = @class_name || raise CustomError.new if node.name == "initialize" name = "new" method_receiver = "#{@class_queue.last}.class" return_type = @class_queue.last method_receiver = "#{class_name}.class" return_type = class_name else name = node.name method_receiver = node.receiver ? "#{@class_queue.last}.class" : @class_queue.last method_receiver = node.receiver ? "#{class_name}.class" : class_name end

@class_name is assigned the value of the generate_classname which will be an empty string if the queues are empty.

property class_name : String?

and

if this_class_name = class_name # Use this_cass_name here... end

WDYT?

Although I see no issues with initializing the class_name = ""

@bew Is this a bad practice or something? 😅

I'll simply say:

method_receiver = node.receiver ? "#{@class_name}.class" : @class_name

In this case, does it make sense (allowed) to have @class_name as an empty string?
I don't think so, so it must be made difficult/impossible to do.

that won't happen cause if class_name is "" then class_queue is empty and in here we opt out

scry/src/scry/completion/method_db.cr

Line 73 in d05348c

return false if @class_queue.empty? || node.visibility != Crystal::Visibility::Public

he idea of the classname is just to cache the output of generate_class_name which never returns nil, so initialising it as nil doesn't seem like the right way to handle it.

@laginha87 Oh, ok, I think class_name = "" is not that bad @bew WDYT?

yeah let's not block the PR for a tiny non-straight-forward thing like that, it's ok

bew · 2018-05-05T16:44:28Z

src/scry/completion/method_db.cr

-      @classes[node.name.to_s] = [] of MethodDBEntry
-      @class_queue << node.name.to_s
+      @class_queue << node.name
+      @class_name = generate_name(@module_queue + @class_queue)


Does this still work if you have a module inside a class inside a module?

Why are you separating module & classes queues?

oh, I didn't even know that could happen

It's seperate cause we only store methods for class, for now.

@bew I tried to change this to use classes and modules in the same queue but started getting some tests failing not really sure why. I cut a new branch to add this and module functions as well.

can you create a new issue to track and not forget this? It's quite important
thanks

Also, include what you tried and your results, it might help find the actual problem ;)

I got a pull request https://github.com/crystal-lang-tools/scry/pull/117/files
Should there be a matching issue ?

…om/crystal-lang-tools/scry into laginha87-add-classname-completion

bew · 2018-05-09T21:55:14Z

spec/scry/completion_provider_spec.cr

@@ -136,8 +136,11 @@ module Scry
    context "classname completion" do
      _kind_ = CompletionItemKind::Class
      it_completes("A", %w(Array Atomic ArgumentError AtExitHandlers))
+      it_completes(" ::A", %w(Array Atomic ArgumentError AtExitHandlers))
+      it_completes(" ::", %w())


why nothing? It should give every top level classes & constants

@bew I think listing all classes would be very slow 😅 , no?

By example on Workspace symbols, if query is empty, you get this:

@faustinoaq you're showing symbols here, not completion.

Slow? I don't know.

So for now it only gives an empty list? Maybe that's ok

I couldn't figure out why it didn't return anything :(

No problem, I think is ok 👍

@laginha87 let's not block this pr as it's not so important, but please open an issue to keep track of :: completion, and fix it one day, or decide what we do about it

I actually wanna tackle doing a context aware comlpetion for this at some point .
for example:

module A ::

should complete with modules and classes inside A as well

@bew added #118

@laginha87 there is no need for contextual completion, :: completions will ALWAYS be the top level things, that's what it exists for, there is no contextual things going on for types that starts with ::

faustinoaq · 2018-05-13T22:01:40Z

@crystal-lang-tools/scry Can we get another review here?

laginha87 · 2018-05-18T08:32:24Z

@bew, thanks for reviewing the code, can you approve this or is there something I missed?

bew

Looks good overall 😃

bew · 2018-05-18T09:11:38Z

spec/scry/completion_provider_spec.cr

                      a.", %w(method)
      end
    end
+
+    context "classname completion" do


Note: it can also complete module names (and structs), not only class names, maybe change the spec name?

bew · 2018-05-18T09:19:44Z

spec/scry/completion_provider_spec.cr

+
+    context "classname completion" do
+      _kind_ = CompletionItemKind::Class
+      it_completes("A", %w(Array Atomic ArgumentError AtExitHandlers))


This means that this spec rely on external code... A change to the stdlib, an addition/removal of a class starting with A will break this spec...

There are other problems with how the specs currently works, like:

You can't add a description of what that spec is actually spec-ing

They rely on external code (see above)

They are not self-contained: by just reading the spec you don't directly have all the data to understand it (you have to always lookup other files, AND they are shared between many specs)

This should probably go to a new issue though. I'm just too tired right now ^^

The test will always break with changes to stdlib, we are testing the autocomplete on it I don't think it's something we should worry about.

What other files do you have to check to understand the specs? My idea for this file was to have tests that almost did an end-to-end test on the completion features and not dependant on the actual implementation.

bew · 2018-05-18T09:23:13Z

src/scry/completion/class_module_context.cr

+    def find
+      target_header_index = @target.rindex(":")
+      @method_db.type_match(@target).map do |label|
+        label = label[(target_header_index + 1)..-1] if target_header_index


just a style comment: At first I didn't saw the if at the end of the line, and was wondering 'how the hell does that compile..."

I think that line is too 'complicated' / 'long' for a suffix if, and would suggest to make it a multi-line if.

bew · 2018-05-18T09:25:27Z

src/scry/completion/class_module_context.cr

+          label: label,
+          insert_text: label,
+          kind: CompletionItemKind::Class,
+          detail: label,


would it be better to have the full path (e.g: JSON::Any, not just Any) in details? (can someone test this, see how it goes?)

you mean in the CompletionItem class ?

bew · 2018-05-18T09:28:26Z

src/scry/completion/method_db.cr

@@ -20,6 +20,10 @@ module Scry::Completion
      end.select(&.name.starts_with? text).to_a
    end

+    def type_match(text : String) : Array(String)
+      @db.keys.select!(&.starts_with?(text)).reject!(&.ends_with?(".class"))


@db.keys.select! { |k| k.starts_with?(text) && !k.ends_with?(".class") }

? (less allocations / simpler logic / ...)

laginha87 · 2018-05-29T08:43:04Z

@bew thanks for the review 👍
I addressed most of the issues you pointed out.
What you brought up about the test is valid but think it should be taken care of in another PR.
Didn't understand what you meant by "would it be better to have the full path (e.g: JSON::Any, not just Any) in details? (can someone test this, see how it goes?)"
Can you take another look and maybe approve 😅?

bew · 2018-05-30T18:20:34Z

Actually it's not important

bew · 2018-05-30T18:21:34Z

spec/fixtures/completion/sample.cr

@@ -1,6 +1,6 @@
 require "./tree"

-class A
+class B


By "looking another file to understand the specs" I meant this file!

faustinoaq · 2018-05-30T19:17:24Z

@laginha87 Thank you! 🎉

laginha87 added 6 commits May 4, 2018 20:09

Adds failing specs

e486850

Adds classname and module completion

8338024

Fix formating issues

7232527

Sets ulimit on travis

b1aea16

Increase the ulimit more

017b367

Removes ulimit call

c6ac56e

faustinoaq requested a review from a team May 4, 2018 21:51

faustinoaq added enhancement review required labels May 4, 2018

Merge branch 'master' into laginha87-add-classname-completion

d05348c

faustinoaq approved these changes May 5, 2018

View reviewed changes

faustinoaq requested review from kofno, keplersj, bmulvihill and bew May 5, 2018 14:03

bew reviewed May 5, 2018

View reviewed changes

laginha87 added 3 commits May 9, 2018 19:24

Use ! version of methods for less memory usage

ebc94ad

Adds more specs

bf8fe60

Merge branch 'laginha87-add-classname-completion' of https://github.c…

5452313

…om/crystal-lang-tools/scry into laginha87-add-classname-completion

bew reviewed May 9, 2018

View reviewed changes

This was referenced May 10, 2018

Laginha87 add module functions #117

Closed

Handle :: classname autocompletion #118

Closed

bew reviewed May 18, 2018

View reviewed changes

Applies cr feedback

8e91d84

laginha87 force-pushed the laginha87-add-classname-completion branch from 0d319ea to 8e91d84 Compare May 26, 2018 10:51

faustinoaq requested a review from bew May 29, 2018 12:18

bew reviewed May 30, 2018

View reviewed changes

bew approved these changes May 30, 2018

View reviewed changes

faustinoaq merged commit 7ea71f9 into master May 30, 2018

faustinoaq deleted the laginha87-add-classname-completion branch May 30, 2018 19:17

faustinoaq removed the review required label Jun 14, 2018

Add class and module name completion #100

Add class and module name completion #100

Conversation

laginha87 commented May 4, 2018

faustinoaq commented May 4, 2018

laginha87 commented May 5, 2018

faustinoaq left a 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 8, 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

laginha87 May 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew May 10, 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

faustinoaq commented May 13, 2018

laginha87 commented May 18, 2018

bew left a 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

Choose a reason for hiding this comment

laginha87 commented May 29, 2018

bew commented May 30, 2018

Choose a reason for hiding this comment

faustinoaq commented May 30, 2018

bew May 8, 2018 •

edited

Loading

laginha87 May 9, 2018 •

edited

Loading

bew May 9, 2018 •

edited

Loading

bew May 10, 2018 •

edited

Loading