Add param completion command #30

rojepp · 2015-06-22T21:35:25Z

This is a starting point for #29.

I'm not sure I'm happy with the serialized format, see added output.json.

Some intergration tests currently fail, will investigate.

rojepp · 2015-06-22T21:37:05Z

FSharp.AutoComplete/Program.fs

@@ -366,7 +370,7 @@ module internal Main =
     new RangeConverter() :> JsonConverter
    |]

-  let prAsJson o = printAgent.WriteLine (JsonConvert.SerializeObject(o, jsonConverters))
+  let prAsJson o = printAgent.WriteLine (JsonConvert.SerializeObject(o, Formatting.Indented,  jsonConverters))


This prints indented, instead of trying to indent later in integration tests. Hope this is ok?

I'm afraid not really. The JSON parsers that interpret what is printed by FSAC assume that each message is exactly one line.

I've reverted this.

rneatherway · 2015-06-23T09:17:16Z

FSharp.AutoComplete/Program.fs

@@ -607,6 +611,21 @@ module internal Main =

            main state

+
+          | Methods ->
+            let meth = tyRes.GetMethods(line, col, lineStr)


I think at this point we should first scan back using this code. An editor should be able to give the current cursor position with some parameters already filled in.

I wonder on this point, though, with multiple parameters how we could communicate to the editor which parameter we are on.

Maybe something to consider when deciding on a json format. Which parameter of which overload.

rneatherway · 2015-06-23T09:26:10Z

I think the test should have some overloads and multiple parameters. Perhaps something like:

type NewObjectType() =

  member x.Terrific (y : int) (z : char) : int =  y
  member x.Terrific (y : int) (z : System.DateTime) : int =  y
  member x.Terrific (y : Set<'a>) (z : int) : Set<'a> =  y

And maybe using something from the standard library like System.DateTime.Parse. I could send a PR to this PR if that'll help?

rneatherway · 2015-06-23T09:27:17Z

I agree, I don't think the structure with tuples and ADTs has serialized well at all. We should see how it looks in a more complicated case and come up with a record and list structure that can be consumed cleanly.

rojepp · 2015-06-23T09:31:11Z

Please do send a PR if you'd like to help out!

rojepp · 2015-06-23T09:44:43Z

I'd appreciate a test that fails because the startOffset code is not there.

Test with overloaded methods and request in call

rojepp · 2015-06-25T18:47:12Z

@rneatherway I've incorporated the startPos code from fsharpbinding, and the tests pass.
What are your thoughts on the output format?

rneatherway · 2015-06-25T19:16:07Z

Thanks, I will have a think about it tomorrow. I definitely think we should define a simple type hierarchy to return the results in.

rojepp · 2015-06-25T19:25:49Z

Given

{
  "Kind": "method",
  "Data": {
    "Item1": "NewObjectType",
    "Item2": [
      {
        "Description": {
          "Case": "FSharpToolTipText",
          "Fields": [
            [
              {
                "Case": "Group",
                "Fields": [
                  [
                    {
                      "Item1": "new : unit -> FileTwo.NewObjectType",
                      "Item2": {
                        "Case": "None"
                      }
                    }
                  ]
                ]
              }
            ]
          ]
        },
        "TypeText": ": FileTwo.NewObjectType",
        "Parameters": [],
        "IsStaticArguments": false
      }
    ]
  }
}

type OverloadParameter =
  {
    ParameterName : string
    Name : string
    CanonicalTypeTextForSorting : string
    Display : string
    Description : string
  }
type Overload = 
  {
    Tip : string
    TypeText : string
    Parameters : OverloadParameter list
    IsStaticArguments : bool
  }
type MethodResponse =
  {
    Name : string
    Overloads : Overload list
  }

There are some assumptions here: Description always returns FSharpToolTipText DU case, Fields always returns DU case Group. I have no idea if these assumptions hold. :) This is as flat as I could get it without losing any information. I don't presume to know exact uses of this, so I'd rather not leave anything out.

There may be more to do, but now it looks pretty usable.

rojepp · 2015-06-25T21:34:47Z

@rneatherway There's a deliberate serialization in place. It may need improvements, but it looks pretty ok to me. I didn't add XmlDoc resolution, I consider that a separate issue. There are now two places where that should be done.

rneatherway · 2015-06-26T13:42:12Z

If your assumption is just on the tooltip text, then it seems reasonable to assume that passing it to TipFormatter.formatTip should account for the different cases there. It's interesting, I suppose if we do want to allow editors to produce a richer tooltip output then we will want to stop called that function and return the fully structured information somehow.

rneatherway · 2015-06-26T15:36:37Z

FSharp.AutoComplete/test/integration/ParamCompletion/ParamCompletionRunner.fsx

+p.parse "Program.fs"
+Threading.Thread.Sleep(6000)
+p.methods "Program.fs" 4 36
+p.methods "Program.fs" 4 37


This one is failing, but I think it shouldn't. Using 1-indexed columns this is a request at $ in let testval = FileTwo.NewObjectType($), so I think we should return results here. The previous one is actually on the open bracket, which it would be nice if it succeeded, but I think slightly less important than this one.

Indeed, I'd go so far as to say the first one shouldn't succeed. :)

rneatherway · 2015-06-26T15:51:38Z

Looking good, but I'd like to sort out this off-by-1 issue. They can be really annoying to debug at the level of the editor. The other thing is, I wonder if we could add a field to MethodResponse called Parameter: int, which indicates what parameter we are on. This would be helpful for an editor and would require extending the scanning-back loop to track the number of commas seen at the current depth.

rojepp · 2015-06-26T16:47:54Z

Great feedback, thanks. I'll try and make some time to address your concerns. We should definitely bail at some arbitrary cutoff, 3 lines or so. :)

rojepp · 2015-06-27T07:39:16Z

I've addressed all concerns except this one:

I suppose if we do want to allow editors to produce a richer tooltip output then we will want to stop called that function and return the fully structured information somehow.

Do you have a suggestion for how this might look?

rneatherway · 2015-06-27T08:07:43Z

Great, thanks! I will have time to review and merge this tomorrow. At a quick glance it all looks good to me. Regarding the richer tooltip output, I don't think that is important for this change. I was just thinking of offering the option for this and the normal tooltips of returning structured data rather than a preformatted string.

7sharp9 · 2015-06-27T16:17:33Z

If you find any bugs with the source from the F# addin please PR back upstream, there are quite a few edge cases that probably are not covered.

Add param completion command

rojepp added 4 commits June 22, 2015 16:48

Methods WIP

42c5933

Some intergration tests currently fail, will investigate.

Fix integration tests

dfa209b

Fix test diffs on my machine by sorting references on filename only

2cc2cc0

Add missing output.json

5fb093f

rojepp reviewed Jun 22, 2015
View reviewed changes

Revert json indent settings

05810ff

rneatherway reviewed Jun 23, 2015
View reviewed changes

Guard for info.Length = 0

12c1a2a

rneatherway and others added 3 commits June 23, 2015 11:09

Test with overloaded methods and request in call

ee564f2

Merge pull request #1 from rneatherway/methods

6690c7a

Test with overloaded methods and request in call

Incorporate finding of start position from fsharbinding

0d3c696

Improved json format for methods.

67d935a

There may be more to do, but now it looks pretty usable.

rneatherway reviewed Jun 26, 2015
View reviewed changes

rojepp added 2 commits June 27, 2015 09:15

Fix off-by-one and add cutoff to avoid searching through the whole file

28d7b78

Report current parameter index by counting commas at depth 0.

a06b9d8

rneatherway added a commit that referenced this pull request Jun 28, 2015

Merge pull request #30 from rojepp/methods

bfd4c7d

Add param completion command

rneatherway merged commit bfd4c7d into ionide:master Jun 28, 2015

rojepp deleted the methods branch June 28, 2015 21:34

rneatherway mentioned this pull request Jun 30, 2015

New command for parameter completions #29

Closed

rneatherway pushed a commit that referenced this pull request Dec 20, 2016

support msbuild15, same as preview2 (#30)

c19957d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add param completion command #30

Add param completion command #30

rojepp commented Jun 22, 2015

rojepp Jun 22, 2015

rneatherway Jun 23, 2015

rojepp Jun 23, 2015

rneatherway Jun 23, 2015

rneatherway Jun 23, 2015

rojepp Jun 23, 2015

rneatherway commented Jun 23, 2015

rneatherway commented Jun 23, 2015

rojepp commented Jun 23, 2015

rojepp commented Jun 23, 2015

rojepp commented Jun 25, 2015

rneatherway commented Jun 25, 2015

rojepp commented Jun 25, 2015

rojepp commented Jun 25, 2015

rneatherway commented Jun 26, 2015

rneatherway Jun 26, 2015

rojepp Jun 27, 2015

rneatherway commented Jun 26, 2015

rojepp commented Jun 26, 2015

rojepp commented Jun 27, 2015

rneatherway commented Jun 27, 2015

7sharp9 commented Jun 27, 2015

Add param completion command #30

Add param completion command #30

Conversation

rojepp commented Jun 22, 2015

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

rneatherway commented Jun 23, 2015

rneatherway commented Jun 23, 2015

rojepp commented Jun 23, 2015

rojepp commented Jun 23, 2015

rojepp commented Jun 25, 2015

rneatherway commented Jun 25, 2015

rojepp commented Jun 25, 2015

rojepp commented Jun 25, 2015

rneatherway commented Jun 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rneatherway commented Jun 26, 2015

rojepp commented Jun 26, 2015

rojepp commented Jun 27, 2015

rneatherway commented Jun 27, 2015

7sharp9 commented Jun 27, 2015