Add autocompletion to module imports. #46

laginha87 · 2018-02-07T19:36:17Z

I set up the groundwork to add autocompletion to scry.
Autocompletion needs the files in memory and not the saved files so I had to add a bunch of code to handle that.
Since I'm not super familiar with writing visitors and stuff I though I'd add autocompletion to the require "" statements because scry's autcompletion can work at the same time as the vscode crystal extension.

Related to #28

laginha87 · 2018-02-07T19:39:04Z

shard.yml

@@ -7,3 +7,7 @@ authors:
  - Faustino Aguilar <http://faustinoaq.github.io>

 license: MIT
+
+targets:


I added this so scry can be built by doing shards build

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

This is awesome! @laginha87 Thank you so much!!! 😄

faustinoaq · 2018-02-07T23:06:22Z

I gonna take some time to review this, I'll also push a fix for Travis CI (started on #44)

faustinoaq · 2018-02-25T06:18:44Z

Hi @laginha87 I tested your PR and require completion works super nice 👍

However, something in this PR is breaking other features like word and snippet completion. Maybe this happens because still isn't full autocompletion and currently just works for module imports.

In this screenshot you can't use def snippet because completion is Loading...

This PR is being awesome! 👏 🎉 😄

faustinoaq · 2018-02-25T06:34:41Z

Oh! this works pretty nice with local modules as well 😄

faustinoaq · 2018-02-25T06:55:19Z

Maybe we can find a way to include symbols on auto-completion, like I did here

Also, if no autocompletion is found, we can try to suggest words and snippets:

WDYT?

laginha87 · 2018-02-25T12:41:45Z

The issue was happening here I was raising an exception when I couldn't identify the kind of autocompletion to apply. The exception was uncaught and then vscode would wait for the response which would never show up.
I changed it to respond with an empty array of completion items when that happens, this way it includes the snippets and symbols from the extension.
I haven't looked at all of the spec for a language server but should snippets be part of it or should they come from the extension that connects to the language server ?

laginha87 · 2018-02-25T12:48:11Z

Another thing that may cause the Loading... issue is the lack of concurrency in scry. Because scry processes requests sequentially there are tasks that are run when you open a file that you to have to wait for before getting a response for autocompletion.

I don't know how we can get around that, we could spawn a Fiber on every request but I think we'd need to change the way we wait for input cause it doesn't seem like it yields to other fibers.

faustinoaq · 2018-02-25T14:39:54Z

Hi @laginha87 I tried your last changes and looks like Loading... issue is fixed 👍

However, I modified your code a bit and seems like CancelParams isn't needed.

I haven't looked at all of the spec for a language server but should snippets be part of it or should they come from the extension that connects to the language server ?

The extension code that connect to LSP is pretty simple, just a few line of codes.
I think it can't manage that behavior.

faustinoaq · 2018-02-25T14:44:44Z

Another thing that may cause the Loading... issue is the lack of concurrency in scry. Because scry processes requests sequentially there are tasks that are run when you open a file that you to have to wait for before getting a response for autocompletion.

Yeah, I thought that, so, I tried using concurrent channels: (I'll open a new PR about this)

    next_request = Channel(Nil).new
    context = Context.new

    loop do
      spawn do
        content = Request.new(STDIN).read
        request = Message.new(content).parse
        results = context.dispatch(request)
      rescue ex
        results = [ResponseMessage.new(ex)]
      ensure
        response = Response.new([results].flatten)
        response.write(STDOUT)
        next_request.send nil
      end
      select
      when next_request.receive
        Log.logger.info("Scry has processed a request!")
        next
      else
        sleep 1
      end
    end

And Loading... issue was still happening because it was raising a Completion error.

Fixed when it returns an empty array.

faustinoaq · 2018-02-25T14:53:14Z

BTW, Please, sync the master branch on this PR, I can't see update button, I don't know why 😅

faustinoaq

This PR looks very nice, waiting for some more comments @kofno @bcardiff @keplersj 👀
Also Travis is failing now 😅
BTW, Thank you for adding code base for method and instance var completion 👍

… completion items and not arrays

laginha87 · 2018-02-25T19:49:32Z

I updated the branch with master and fixed the tests.
The cancel request not being handled I don't think it causes issues but if we don't handle it it'll raise an exception and show an error in the developer tools.

bmulvihill · 2018-02-26T00:45:01Z

spec/scry/symbol_spec.cr

@@ -19,39 +19,39 @@ module Scry
      text_document = TextDocument.new("uri", ["class Test; end"])


Any reason we are updating symbol specs in this PR? Seems unrelated.

Because of the way response message https://github.com/crystal-lang-tools/scry/pull/46/files#diff-82346a49d1a2a5c31d9a3445d7a9127dR9 I had to add a single CompletionItem as a response for the completion resolver.
The symbol specs expect an array in the response.

faustinoaq

I approve this, someone else?

keplersj

Just did a quick skim-through. No objections from me

bew · 2018-02-26T23:49:42Z

src/scry/completion/require_module_context.cr

+
+    def find
+      case @import
+      when RELATIVE_IMPORT_REGEX


when .starts_with? '.' would be faster here I think (and less regex is almost always better 😄 )

laginha87 · 2018-02-28T12:33:19Z

Thanks for all the feedback.
I applied the change to not use a regex.
Is this ready to be merged?

faustinoaq · 2018-02-28T12:52:56Z

Is this ready to be merged?

Yeah, I think it is ready, Thank you so much for this PR 😄 👍

@keplersj I'm working on concurrent rpc for scry, I think we should publish a new release after that, WDYT?

keplersj · 2018-02-28T15:02:52Z

@faustinoaq 👍. In truth I'd be okay with configuring something like semantic-release for scry.

laginha87 changed the title ~~28 add autocompletion~~ Add autocompletion to module imports. Feb 7, 2018

laginha87 commented Feb 7, 2018

View reviewed changes

keplersj requested a review from faustinoaq February 7, 2018 19:40

faustinoaq mentioned this pull request Feb 8, 2018

Fix nodejs diagnostics (#47) crystal-lang-tools/vscode-crystal-lang#48

Merged

laginha87 force-pushed the 28-add-autocompletion branch from 6621ec0 to 13d1ddf Compare February 22, 2018 19:22

faustinoaq reviewed Feb 25, 2018

View reviewed changes

This was referenced Feb 25, 2018

Implements concurrent requests #48

Closed

Add completion to roadmap #49

Merged

laginha87 added 12 commits February 25, 2018 19:43

Add necessary structs for Completion Protocol

05e6dee

Use base of cracker code for generating a db for completion

9621d2e

Add more stuff for the server language protocol

7174627

Add server capabilities

8a6ef0c

Add the completion item to the protocol definition

f9752ce

Adds response

7f57c5b

Properly set the completion provider options

9729a94

Get completion working

357aec4

Refactor completion_params out

eee50f0

WIP

b79f8e1

Refactors some use cases

d846776

Adds completion of modules

208ce7a

laginha87 added 10 commits February 25, 2018 19:43

Adding docs WIP

f955267

Adds relative module load

c1f4069

Refactor WIP

02e93c1

Improve module completion

02bd5fd

Improve label on files

97138a1

Adds a completion resolver spec

54696df

Adds spec for module

6f8946a

Format code

1e74e4b

Rename key to snakecase

5c4ae11

Adds cancel request support, and stops breaking on unidentified context

5e2ba8a

laginha87 force-pushed the 28-add-autocompletion branch from 5e705f2 to 5e2ba8a Compare February 25, 2018 19:44

Add cast to array in tests because completion resolver returns single…

97fe704

… completion items and not arrays

bmulvihill reviewed Feb 26, 2018

View reviewed changes

faustinoaq approved these changes Feb 26, 2018

View reviewed changes

keplersj approved these changes Feb 26, 2018

View reviewed changes

bew reviewed Feb 26, 2018

View reviewed changes

Use .starts_with? instead of regex to match relative imports

2ad9c9b

faustinoaq merged commit 0a3627e into crystal-lang-tools:master Feb 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add autocompletion to module imports. #46

Add autocompletion to module imports. #46

laginha87 commented Feb 7, 2018

laginha87 Feb 7, 2018

faustinoaq commented Feb 7, 2018

faustinoaq commented Feb 7, 2018

faustinoaq commented Feb 25, 2018 •

edited

Loading

faustinoaq commented Feb 25, 2018

faustinoaq commented Feb 25, 2018

laginha87 commented Feb 25, 2018

laginha87 commented Feb 25, 2018 •

edited

Loading

faustinoaq commented Feb 25, 2018

faustinoaq commented Feb 25, 2018

faustinoaq commented Feb 25, 2018

faustinoaq left a comment •

edited

Loading

laginha87 commented Feb 25, 2018

bmulvihill Feb 26, 2018

laginha87 Feb 26, 2018

faustinoaq left a comment

keplersj left a comment

bew Feb 26, 2018

laginha87 commented Feb 28, 2018

faustinoaq commented Feb 28, 2018

keplersj commented Feb 28, 2018

		@@ -19,39 +19,39 @@ module Scry
		text_document = TextDocument.new("uri", ["class Test; end"])

Add autocompletion to module imports. #46

Add autocompletion to module imports. #46

Conversation

laginha87 commented Feb 7, 2018

laginha87 Feb 7, 2018

Choose a reason for hiding this comment

faustinoaq commented Feb 7, 2018

faustinoaq commented Feb 7, 2018

faustinoaq commented Feb 25, 2018 • edited Loading

faustinoaq commented Feb 25, 2018

faustinoaq commented Feb 25, 2018

laginha87 commented Feb 25, 2018

laginha87 commented Feb 25, 2018 • edited Loading

faustinoaq commented Feb 25, 2018

faustinoaq commented Feb 25, 2018

faustinoaq commented Feb 25, 2018

faustinoaq left a comment • edited Loading

Choose a reason for hiding this comment

laginha87 commented Feb 25, 2018

bmulvihill Feb 26, 2018

Choose a reason for hiding this comment

laginha87 Feb 26, 2018

Choose a reason for hiding this comment

faustinoaq left a comment

Choose a reason for hiding this comment

keplersj left a comment

Choose a reason for hiding this comment

bew Feb 26, 2018

Choose a reason for hiding this comment

laginha87 commented Feb 28, 2018

faustinoaq commented Feb 28, 2018

keplersj commented Feb 28, 2018

faustinoaq commented Feb 25, 2018 •

edited

Loading

laginha87 commented Feb 25, 2018 •

edited

Loading

faustinoaq left a comment •

edited

Loading