Don't show empty parameter XML Doc in signature helper and correctly compute argument index #1895

cartermp · 2016-11-29T23:34:02Z

Fixes #1893:

However, there is also another bug - only the first parameter is shown when typing another parameter:

This is the same behavior in master:

I can take a look, but meanwhile this does address the issue of showing empty parameter XML doc comments.

…nts are defined

KevinRansom

Nice one Phillip, looks good

cartermp · 2016-11-30T02:37:10Z

Looks like argumentIndex is evaluated to 0 when typing a comma after the first argument, which is the cause of the second two screenshots. If I force it to be 1, it highlights the correct parameter, so that seems to be the source of the error. I also noticed that it will sometimes evaluate to 1 correctly, but only after tying the whole expression, deleting up until the comma, clicking there, deleting the comma, and then pressing , again.

In short, I think it's buggy.

dsyme · 2016-11-30T08:17:21Z

Yeah, there's a -1 on the argument index which may be going awry. It's there for.the case where "(" is the start of a zero-argument list "()". We can report the argument index in the package of data handed off to unit testing.

dsyme · 2016-11-30T14:33:56Z

vsintegration/src/FSharp.Editor/SignatureHelp.fs

@@ -155,17 +155,22 @@ type FSharpSignatureHelpProvider [<ImportingConstructor>]  (serviceProvider: SVs

        for method in methods do
            // Create the documentation. Note, do this on the background thread, since doing it in the documentationBuild fails to build the XML index
-            let methodDocs = XmlDocumentation.BuildMethodOverloadTipText(documentationBuilder, method.Description, true)
+            let methodDocs = XmlDocumentation.BuildMethodOverloadTipText(documentationBuilder, method.Description, false)


I believe we still want true here

with "true" the active parameter is shown underneath the list of all parameters. The active parameter is shown in bold. There is duplication.

with "false", only one the active parameter is shown, no others

The duplication is unfortunate and seems a Roslyn proble? But I think we definitely want all parameters - more information is better than less here.

This is the behavior that C# exhibits:

And when hovering the member:

It seems like we're a bit more spacey than C# is, but they don't show all parameters + the active parameter when hitting (.

I guess I should clarify - I believe that the right behavior here is the one that C# does (and that this PR does as well). Showing all parameters could be too much on the eyes for any method with more than 3 parameters. I don't have data proving this, but I believe that there are enough cases out there that justify this.

dsyme · 2016-12-01T12:02:26Z

I guess I should clarify - I believe that the right behavior here is the one that C# does (and that this PR does as well). Showing all parameters could be too much on the eyes for any method with more than 3 parameters. I don't have data proving this, but I believe that there are enough cases out there that justify this.

It's tricky. We've shown all the parameters for a long time, so I'm used to it. Also (and I think crucially), VIsual F# just doesn't have the same full set of navigational/library-lookup mechanisms as C#, so this is the only way an F# user gets information, short of looking up the docs.

We went around a similar loop with QuickInfo displays for VF# 2.0. F# shows much more info than C#. We thought about changing to C#'s version, but soon discovered that you could just no longer find out anything from F#.

So my gut feeling is that we should show at least what we showed in VS2008-15, which is all the parameters. Showing less is prettier, but leaves the user in a very awkward place if they want to get more information. At the moment even F1 help to go to MSDN is not wired up.

cartermp · 2016-12-01T16:46:31Z

@dsyme Okay, How about I revert the call the BuildMethodOverloadTipText with false and create an issue with what I think the behavior should be.

I think that the current behavior (hovering over something only provides the summary, invoking that QuickInfo item with (, <, ,) should be flipped. I can provide some examples of what that looks like.

dsyme · 2016-12-01T17:47:12Z

ok

cartermp · 2016-12-02T21:25:51Z

@dsyme I think I figured out the argument index problem.

It works as expected for all of these. Deleting and re-adding a comma or open paren also works:

type Foo() =
    /// <summary>
    /// Does even more stuff.
    /// </summary>
    member this.DoAnotherThing(aString, anInt, aFloat) =
        sprintf "The string is: %s, and the int is: %d, and the float is: %f" aString anInt aFloat

    /// <summary>
    /// Does other stuff with lots of parameters.  It really is a lot of stuff and this is a fairly long summary comment.
    /// </summary>
    /// <param name="aString">This is the string which gets passed into this method.  This is kind of a long param comment, but not uncommon.</param>
    /// <param name="anInt">This is the integer which gets passed into this method.  This is kind of a long param comment, but not uncommon.</param>
    /// <param name="aFloat">This is the floating-point value which gets passed into this method.  This is kind of a long param comment, but not uncommon.</param>
    /// <param name="anArray">This is the array which gets passed into this method.  This is kind of a long param comment, but not uncommon.</param>
    /// <exception cref="System.Exception">Thrown when the exception is thrown.</exception>
    /// <exception cref="System.ArgumentException">Thrown when the exception is thrown.</exception>
    /// <exception cref="System.ArgumentNullException">Thrown when the exception is thrown.</exception>
    /// <exception cref="System.Aggregate">Thrown when the exception is thrown.</exception>
    member this.HasManyParameters(aString, anInt, aFloat, anArray: 'a[]) =
        sprintf "The string is: %s, and the int is: %d, and the float is: %f, and the array is: %A" aString anInt aFloat anArray
    
    /// <summary>
    /// Does some stuff.
    /// </summary>
    member this.DoStuff(aString, anInt) =
        sprintf "The string is: %s, and the int is: %d" aString anInt

    /// <summary>
    /// Does other stuff.
    /// </summary>
    /// <param name="aString">A string!</param>
    /// <param name="anInt">An int!</param>
    member this.DoOtherStuff(aString, anInt) =
        sprintf "The string is: %s, and the int is: %d" aString anInt

dsyme · 2016-12-03T17:23:55Z

It looks like the unit tests need updating

1) Failed : Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.SignatureHelpProvider.ShouldGiveSignatureHelpAtCorrectMarkers
FSharpCompletionProvider.ProvideMethodsAsyncAux() gave unexpected results, expected Some ("[7..40)", 0, 2, None), got Some ("[7..40)", 1, 2, Some "arg1")

If you need to, follow the instructions at the top of the Signature Help test file to run the tests in F# Interactive and dump the update expected results.

dsyme · 2016-12-03T17:24:04Z

Fix looks good otherwise!!

cartermp · 2016-12-03T17:30:48Z

Ah! I'll update that test (and perhaps add a few more test cases).

dsyme · 2016-12-03T17:45:56Z

@cartermp Great! :)

cartermp · 2016-12-03T23:52:44Z

I modified the first test case a bit and added another expected clause. The rest were just adjustments to the expected span which comes back from ProvideMethodsAsyncAux.

dsyme · 2016-12-04T02:08:03Z

Close/reopen to run tests again

vasily-kirichenko · 2016-12-04T07:26:40Z

@cartermp does this PR fix the following duplication?

cartermp · 2016-12-04T18:13:56Z

@vasily-kirichenko Technically yes, but it's still not that great of a solution. The best we can do is to change this line to pass in [||] instead of doc, and we won't get Roslyn-based parameter info at the bottom. However, the active parameter still won't be highlighted. I'm inclined not to do this because I prefer duplication over not highlighting the active parameter.

Alternatively, we would request a Roslyn feature to understand the way that we currently display all parameters, and highlight the active one based on that. I think it's very unlikely that such a change would be accepted at this time.

KevinRansom · 2016-12-05T23:26:28Z

@dsyme @vasily-kirichenko

We should remove the duplicated line and fix f1 and f12 to allow us to show the additional information.
@cartertmp Please remove the duplicated information and ensure there is an issue trcking the extra work.

Kevin

KevinRansom · 2016-12-06T00:34:11Z

@cartermp can you prepare a PR to just remove the redundent line. I will pull this as is.

…compute argument index (dotnet#1895) * Show 'no documentation' as QuickInfo workaround when no XML doc comments are defined * Don't add newline if there aren't parameter docs * Don't ask BuildMethodOverloadTipText to also handle parameters * Show all parameters when invoking signature helper * Account for edge case when computing argument index. * Adjust tests

cartermp added 3 commits November 29, 2016 12:10

Show 'no documentation' as QuickInfo workaround when no XML doc comme…

a46b11d

…nts are defined

Don't add newline if there aren't parameter docs

b712bdf

Don't ask BuildMethodOverloadTipText to also handle parameters

4ae2823

msftclas added the cla-not-required label Nov 29, 2016

KevinRansom approved these changes Nov 30, 2016

View reviewed changes

dsyme reviewed Nov 30, 2016

View reviewed changes

Show all parameters when invoking signature helper

3e4c7d2

cartermp mentioned this pull request Dec 1, 2016

Hover should show all signature info. Consuming something should only show contextual info. #1910

Closed

Account for edge case when computing argument index.

bbd5b8f

cartermp changed the title ~~Don't show empty parameter XML Doc in signature helper~~ Don't show empty parameter XML Doc in signature helper and correctly compute argument index Dec 2, 2016

Adjust tests

203a111

dsyme closed this Dec 4, 2016

dsyme reopened this Dec 4, 2016

msftclas added the cla-not-required label Dec 4, 2016

KevinRansom merged commit 50e7a82 into dotnet:master Dec 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't show empty parameter XML Doc in signature helper and correctly compute argument index #1895

Don't show empty parameter XML Doc in signature helper and correctly compute argument index #1895

cartermp commented Nov 29, 2016

KevinRansom left a comment •

edited

Loading

cartermp commented Nov 30, 2016

dsyme commented Nov 30, 2016

dsyme Nov 30, 2016

cartermp Nov 30, 2016

cartermp Nov 30, 2016

dsyme commented Dec 1, 2016

cartermp commented Dec 1, 2016 •

edited

Loading

dsyme commented Dec 1, 2016

cartermp commented Dec 2, 2016 •

edited

Loading

dsyme commented Dec 3, 2016

dsyme commented Dec 3, 2016

cartermp commented Dec 3, 2016

dsyme commented Dec 3, 2016

cartermp commented Dec 3, 2016

dsyme commented Dec 4, 2016

vasily-kirichenko commented Dec 4, 2016

cartermp commented Dec 4, 2016 •

edited

Loading

KevinRansom commented Dec 5, 2016

KevinRansom commented Dec 6, 2016

Don't show empty parameter XML Doc in signature helper and correctly compute argument index #1895

Don't show empty parameter XML Doc in signature helper and correctly compute argument index #1895

Conversation

cartermp commented Nov 29, 2016

KevinRansom left a comment • edited Loading

Choose a reason for hiding this comment

cartermp commented Nov 30, 2016

dsyme commented Nov 30, 2016

dsyme Nov 30, 2016

Choose a reason for hiding this comment

cartermp Nov 30, 2016

Choose a reason for hiding this comment

cartermp Nov 30, 2016

Choose a reason for hiding this comment

dsyme commented Dec 1, 2016

cartermp commented Dec 1, 2016 • edited Loading

dsyme commented Dec 1, 2016

cartermp commented Dec 2, 2016 • edited Loading

dsyme commented Dec 3, 2016

dsyme commented Dec 3, 2016

cartermp commented Dec 3, 2016

dsyme commented Dec 3, 2016

cartermp commented Dec 3, 2016

dsyme commented Dec 4, 2016

vasily-kirichenko commented Dec 4, 2016

cartermp commented Dec 4, 2016 • edited Loading

KevinRansom commented Dec 5, 2016

KevinRansom commented Dec 6, 2016

KevinRansom left a comment •

edited

Loading

cartermp commented Dec 1, 2016 •

edited

Loading

cartermp commented Dec 2, 2016 •

edited

Loading

cartermp commented Dec 4, 2016 •

edited

Loading