Use paredit in the signature provider to find correct symbol #450

cfehse · 2019-11-04T15:06:21Z

What has Changed?

Changed the method to find the symbol for the info call to use paredit and to retrieve the symbol from the containing ( ) list.
Corrected the handling of 'junk' and 'punk' in forwardSexp() and backwardSexp().

Fixes #410

My Calva PR Checklist

I have:

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

Made sure the PR is directed at the dev branch (unless reasons).
Read the source changes.
Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
- Tested the particular change
- Figured if the change might have some side effects and tested those as well.
- Smoke tested the extension as such.
If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @bpringe

in forwardSexp() and backwardSexp(). Closes #410.

PEZ · 2019-11-04T20:11:19Z

I truly like that you go for the structural way. However, paredit.js is abandonware, and I think we should try get rid of it, rather than build more with it. Could this maybe be done using the LispTokenCursor like we do here: https://github.com/BetterThanTomorrow/calva/blob/master/src/utilities.ts#L91

cfehse · 2019-11-04T20:35:47Z

This can surely be done with the LispTokenCursor. If I have time and the mood I will change that eventuelly - but for now this is better than the current implementation which does not check if the current form is the form for which the symbol can be retrieved.

cfehse · 2019-11-04T20:41:47Z

@PEZ

By the way what is the advantage of the LispTokenCursor over paredit. I find neither of them especially easy to use - as far as I now know both.

PEZ · 2019-11-04T20:50:40Z

I like paredit as such. 😄 What I am objecting to is paredit.js, which is bug ridden and not maintained.

Our own paredit is built with our own LispTokenCursor, which might not always have the perfect API , but it is at least maintained, and much more complete and has fewer bugs than paredit.js. Also, we can always build some nicer abstraction on top of it. A bit like our paredit.ts module does.

I'll see if I have the time to try modify this PR to use our own stuff instead. If not, we'll merge it and fix that later, as you suggest.

cfehse · 2019-11-04T20:55:15Z

Okay I understand. gg

PEZ · 2019-11-04T23:05:09Z

OK. So here is a version using docmirror+LispTokenCursor: 77e5a97

Unless I have misunderstood something, it is equivalent to the paredit.js version. I actually think the TokenCursor API is pretty sweet, at least for this task.

When doing this I noticed one more advantage with the docmirror approach: It avoids building a new AST every time the symbol is fetched.

There is a big risk that I have misunderstood something though, so therefore I am first pushing this in its own branch, so that you can have a look before I update this PR.

WIll have to figure out cropping it. But later.

cfehse · 2019-11-05T07:12:13Z

When doing this I noticed one more advantage with the docmirror approach: It avoids building a new AST every time the symbol is fetched.

For that reason I only builded the AST for the current toplevel form in my approach. But nevertheless your version looks very clean. I will take a look and integrate it. Thanks.

PEZ · 2019-11-05T08:46:24Z

I'll push my changes and then it is ready to merge, I'd say.

…/BetterThanTomorrow/calva into wip/sinature-handler-use-paredit

cfehse · 2019-11-05T09:41:17Z

I'll push my changes and then it is ready to merge, I'd say.

Your code is not doing, what it is supposed to to. Wait - I take a look at this.

PEZ · 2019-11-05T09:42:37Z

Your code is not doing, what it is supposed to to. Wait - I take a look at this.

Too late. 😄

At least I haven't merged the PR yet.

cfehse · 2019-11-05T09:53:25Z

@PEZ

This docMirror.getDocument(document).getTokenCursor(idx); returns the Cursor set to the last token of the last line of the doc regardless where the position is.

cfehse · 2019-11-05T10:02:30Z

@PEZ

I am handle this over to you now - I do not have the time to rework the whole thing. The idea is to find the correct symbol regardless in which place you are in a form.
Examples ( | is the cursor):

(def  | (fn   [1   map  ]  {   } )    ) => def
(def    (fn | [1   map  ]  {   } )    ) => fn
(def    (fn   [1 | map  ]  {   } )    ) => fn
(def    (fn   [1   map  ]  { | } )    ) => fn
(def    (fn   [1   map  ]  {   } ) |  ) => def

Add LispTokenCursor method for searching backwards for a specific list type

PEZ · 2019-11-05T12:02:45Z

Now I think I got it right. At least for the test cases you provided there. Look:

Please help sanity check the LispTokenCursor method I added for helping with the task.

cfehse · 2019-11-05T13:11:38Z

@PEZ

I take a look at that tomorrow. Thanks.

PEZ · 2019-11-05T22:44:34Z

Tomorrow is such a long time away, so I added two more things to the signature help:

Show active parameter.
- Only for regular forms so far.
- It keeps & more active when that is reach and more arguments added.
Show function name in signature help.

It looks like so:

I also made the escape key dismiss the signature hover.

Now using token-cursor instead of regexps.

PEZ · 2019-11-06T14:51:49Z

Now the bugs reported in calva-dev should be fixed. They were about:

argument lists where parameter destruction is going on
ignored forms in the calling argument list
macros with optional arguments (of type arg?). These don't get active parameter markup now.

As far as I can tell, this should be good to go.

cfehse · 2019-11-06T15:31:52Z

@PEZ

Am I missing something? The getSymbol() method of the CalvaSignatureHelpProvider() does not work at all for me. Returns undefined. I pulled this branch from upstream and made a fresh rebuild. I have no idea.

PEZ · 2019-11-06T15:35:42Z

Does the VSIX work for you?

cfehse · 2019-11-06T16:15:42Z

@PEZ

No the signature help will not pop up.

PEZ · 2019-11-06T16:22:18Z

How odd. Could it be a Windows thing...

cfehse · 2019-11-06T16:31:26Z

The VSIX does not work on WSL either.

Now trying native linux...

cfehse · 2019-11-06T16:46:08Z

@PEZ

On native linux the code works. Very strange...

PEZ · 2019-11-06T16:49:52Z

Works on my Windows in Virtual Box...

Is it the same project you use when it works and when it doesn't?

cfehse · 2019-11-06T17:02:24Z

@PEZ

It's not a Windows problem. It is a data problem. I have test data here where it is not working. backwardList() does return false where it should not I think.

PEZ · 2019-11-06T17:04:19Z

Cool. Are you on it now? Otherwise I'd like that data. 😄

cfehse · 2019-11-06T17:09:04Z

@PEZ

The code does not work for me after a '(comment ...)` and under some circumstances in that comment form. It works for the first form in the comment.

cfehse · 2019-11-06T17:14:56Z

@PEZ

Here is some code which confuses the code:

(ns brainpie-test-api.test-case) 

(def )

;; Interactive development
(comment
  (def )
  (while true (do (println "start sleeping")
                  (Thread/sleep 1000)))

  (defn prompt
    "get information from the user"
    [msg]
    (println msg)
    (read-line))
  (prompt "What is my name")

  (go
    (while true
      (println "hello")
      (Thread/sleep 1000)))

  (go
    (def x (atom 1))
    (while (< @x 5)
      (do
        (println @x)
        (Thread/sleep 1000)
        (swap! x inc))))

  (defn fehse
    [x]

    (+ x (+ x 3)))

  (println (fehse))

  (def   (fn [1  map]))

  (defn   [] (map println [1 1 1 1] {1 1} #'- (fehse)))

  )

PEZ · 2019-11-06T18:44:34Z

Works for me with that test data.

cfehse · 2019-11-06T19:10:42Z

@PEZ

On my systems it is not working. Sadly...

PEZ · 2019-11-06T20:51:40Z

So this is a Windows problem after all. At least related. Setting the line ending of the file in VS Code to CRLF will provoke the error. Now the hunt for why this throws things off begins...

We were getting the wrong mapping between offset and rowCol on Windows

PEZ · 2019-11-06T22:09:57Z

Windows/CRLF issue should be fixed now.

Now the only case where I know the wrong argument is marked as active is in thread-first macros, such as ->, and some->. I think I have an idea of how to fix that, but I don't think we need to hold shipping this PR because of that.

PEZ · 2019-11-07T00:58:52Z

And now made it work also when inside thread-first forms.

Here's a demo showing the behaviour working for various test cases:

cfehse · 2019-11-07T09:30:57Z

@PEZ

Now this looks good to me. Very nice feature indeed!

cf added 2 commits November 4, 2019 16:01

Use paredit to find the correct symbol for the signature.

37ed234

Corrected handling of 'junk' and 'punk'

f01d230

in forwardSexp() and backwardSexp(). Closes #410.

PEZ added 4 commits November 5, 2019 00:30

Fix ugly inline eval annotation

b344225

WIll have to figure out cropping it. But later.

Add dimmer demo gif

b983198

Add top-level comment-eval gif [skip ci]

0f65834

Add some demo gifs to README

18ffff1

PEZ and others added 5 commits November 5, 2019 10:34

Crop top-level comment demo gif

0c87dea

Use docmirror token cursor instead of paredit.js

75fc844

Clean up requires

bf8a4c9

Merged branch from upstream.

fb8b15a

Merge branch 'wip/sinature-handler-use-paredit' of https://github.com…

6013781

…/BetterThanTomorrow/calva into wip/sinature-handler-use-paredit

Persist in finding the containing function call

1614c76

Add LispTokenCursor method for searching backwards for a specific list type

PEZ added 4 commits November 5, 2019 22:22

Show active argument in signature help

f4a6148

Make it slightly more readable...

e6912b2

Spelling

0ab50fb

Show function name in signature help

58dbd0f

PEZ added 3 commits November 6, 2019 13:23

Handle destruction argument list parsing

4b3759b

Now using token-cursor instead of regexps.

No parameter help for signatures with opt. args

85cca6f

Honor ignores in function arguments

3dff5f2

Make LineInputModel line ending aware

d1194df

We were getting the wrong mapping between offset and rowCol on Windows

Make active argument inside thread firsts work

2f4c8a1

Merge branch 'dev' into wip/sinature-handler-use-paredit

794ebad

PEZ merged commit 4163833 into dev Nov 7, 2019

PEZ deleted the wip/sinature-handler-use-paredit branch November 7, 2019 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use paredit in the signature provider to find correct symbol #450

Use paredit in the signature provider to find correct symbol #450

cfehse commented Nov 4, 2019 •

edited by PEZ

Loading

PEZ commented Nov 4, 2019

cfehse commented Nov 4, 2019

cfehse commented Nov 4, 2019

PEZ commented Nov 4, 2019

cfehse commented Nov 4, 2019

PEZ commented Nov 4, 2019

cfehse commented Nov 5, 2019

PEZ commented Nov 5, 2019

cfehse commented Nov 5, 2019

PEZ commented Nov 5, 2019 •

edited

Loading

cfehse commented Nov 5, 2019

cfehse commented Nov 5, 2019 •

edited

Loading

PEZ commented Nov 5, 2019

cfehse commented Nov 5, 2019

PEZ commented Nov 5, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019 •

edited

Loading

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

PEZ commented Nov 6, 2019

PEZ commented Nov 7, 2019

cfehse commented Nov 7, 2019

Use paredit in the signature provider to find correct symbol #450

Use paredit in the signature provider to find correct symbol #450

Conversation

cfehse commented Nov 4, 2019 • edited by PEZ Loading

What has Changed?

My Calva PR Checklist

The Calva Team PR Checklist:

PEZ commented Nov 4, 2019

cfehse commented Nov 4, 2019

cfehse commented Nov 4, 2019

PEZ commented Nov 4, 2019

cfehse commented Nov 4, 2019

PEZ commented Nov 4, 2019

cfehse commented Nov 5, 2019

PEZ commented Nov 5, 2019

cfehse commented Nov 5, 2019

PEZ commented Nov 5, 2019 • edited Loading

cfehse commented Nov 5, 2019

cfehse commented Nov 5, 2019 • edited Loading

PEZ commented Nov 5, 2019

cfehse commented Nov 5, 2019

PEZ commented Nov 5, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019 • edited Loading

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

cfehse commented Nov 6, 2019

PEZ commented Nov 6, 2019

PEZ commented Nov 6, 2019

PEZ commented Nov 7, 2019

cfehse commented Nov 7, 2019

cfehse commented Nov 4, 2019 •

edited by PEZ

Loading

PEZ commented Nov 5, 2019 •

edited

Loading

cfehse commented Nov 5, 2019 •

edited

Loading

cfehse commented Nov 6, 2019 •

edited

Loading