Use doccoms from signature and implementation files for quickinfo tooltips #2662

cloudRoutine · 2017-03-20T04:00:51Z

The current way that documentation comments behave with quickinfo tooltips is rather odd and has been a pet peeve of mine for a long time.

When you add doccoms in the signature they don't show up in the corresponding implementation

If you open a different implementation file it uses the doccoms from the sig and overwrites any doccoms you may have added in the implementation file

Now it shows doccoms from the sig in its corresponding implementation file, and if you add additional doccoms in the implementation the tooltip will show both of them.

The doccoms from the implementation files are on the bottom because these are merely supplementary and will not be included in the xml documentation created for the project.

vasily-kirichenko · 2017-03-20T05:14:49Z

Awesome. What about showing a warning of some sort if documentation in sig and implementation files differ (and a quick fix to synchronize them in either way ;) ?

cloudRoutine · 2017-03-20T05:20:46Z

The doccoms in the implementation files are more like "design time" notes, things you want to show up in other implementer's tooltips but prefer not to expose publicly like the doccoms in the signature file.

I don't think synchronization adds anything useful as the doccoms from the signature will always show in the tooltips. If anything you'd want to delete the doccoms in the implementation file. Making them the same would just duplicate the content.

dsyme · 2017-03-20T13:29:58Z

vsintegration/src/FSharp.Editor/Common/Pervasive.fs

+
+type Async with
+    /// Better implementation of Async.AwaitTask that correctly passes the exception of a failed task to the async mechanism
+    static member AwaitTaskCorrect (task:Task) : unit Async =


Async<unit> please.

dsyme · 2017-03-20T13:30:15Z

vsintegration/src/FSharp.Editor/Common/Pervasive.fs

+            |> ignore)
+
+    /// Better implementation of Async.AwaitTask that correctly passes the exception of a failed task to the async mechanism
+    static member AwaitTaskCorrect (task:'T Task) : 'T Async =


Task<'T> and Async<'T> please

dsyme · 2017-03-20T13:31:23Z

vsintegration/src/FSharp.Editor/Common/Pervasive.fs

+type Async with
+    /// Better implementation of Async.AwaitTask that correctly passes the exception of a failed task to the async mechanism
+    static member AwaitTaskCorrect (task:Task) : unit Async =
+        Async.FromContinuations (fun (successCont,exceptionCont,_cancelCont) ->


If this AwaitTask is better, then we should start an RFC to provide a version of it in FSharp.Core please?

@eiriktsarpalis wrote it

dsyme · 2017-03-20T13:32:20Z

vsintegration/src/FSharp.Editor/QuickInfo/QuickInfoProvider.fs

+        let tooltipInfoPair = asyncMaybe {
+            match findSigDeclarationResult with 
+            | FSharpFindDeclResult.DeclNotFound _failReason -> 
+                return (None , Some backupTooltipInfo)


No space before comma please

dsyme · 2017-03-20T13:32:46Z

vsintegration/src/FSharp.Editor/QuickInfo/QuickInfoProvider.fs

+                    let! findImplDefinitionResult = 
+                        checkFileResults.GetDeclarationLocationAlternate
+                            ( idRange.StartLine, idRange.EndColumn, lineText
+                            , lexerSymbol.FullIsland, preferFlag=false) |> liftAsync   


Put comma at end-of-line please, not start of line

dsyme · 2017-03-20T13:34:17Z

vsintegration/src/FSharp.Editor/Common/Pervasive.fs

+
+/// Load times used to reset type checking properly on script/project load/unload. It just has to be unique for each project load/reload.
+/// Not yet sure if this works for scripts.
+let fakeDateTimeRepresentingTimeLoaded x = DateTime(abs (int64 (match x with null -> 0 | _ -> x.GetHashCode())) % 103231L)


Why is this added in this PR? It doesn't seem to be mentioned elsewhere in the PR?

dsyme · 2017-03-20T13:36:31Z

@cloudRoutine An alternative is to fix this within the compiler itself, so that the specification of documentation generation for the language itself changes to combine documentation from signature and implementation. That is preferable in many ways because it solves the problem consistently across all F# tooling including Ionide and Fable.

If implementation documentation is empty then it is uncontroversial to use signature documentation.

However frequently documentation is duplicated in signature and implementation, which raises questions about what to do in that case.

KevinRansom · 2017-03-20T16:11:19Z

build error:

"D:\j\workspace\release_ci_pa---3f142ccc\build-everything.proj" (default target) (1) ->
"D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\fsharp-vsintegration-unittests-build.proj" (Build target) (38) ->
"D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\VisualFSharp.Unittests.fsproj" (Build target) (40) ->
(CoreCompile target) -> 
  D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\QuickInfoProviderTests.fs(102,37): error FS0039: The field, constructor or member 'ProvideQuickInfo' is not defined. [D:\j\workspace\release_ci_pa---3f142ccc\vsintegration\tests\unittests\VisualFSharp.Unittests.fsproj]

    13 Warning(s)
    1 Error(s)

cloudRoutine · 2017-03-20T17:36:13Z

@KevinRansom an issue I've run into with testing new features built around the Roslyn functionality is that there's no easy way to create a workspace to test it out with. Since the default workspace implementations reject all code that isn't C# or vb funtions/methods that rely on Document or Project connections inside of a solution are really hard to test.

cloudRoutine · 2017-03-20T17:49:50Z

@dsyme maybe that can come later, but I'm trying to address a more immediate concern I've had while working on this repo in particular due to the extensive use of .fsi files.

However frequently documentation is duplicated in signature and implementation, which raises questions about what to do in that case.

this is a crude workaround due to inadequate tooling support and I think trying to account for it is going to end up in a lot of generated documentation suddenly becoming very messy. Trying to establish a good heuristic for when doccoms are similar enough to be duplicates seems like an invitation for kludges.

cloudRoutine · 2017-03-22T02:41:01Z

Is there anything else I need to do for this?

cloudRoutine · 2017-03-24T11:51:27Z

this might even encourage people to spend more time writing documentation in signature files since they'd actually get the benefit of it in their implementation files. the giant gross xml doccoms won't have to pollute their code.

cartermp · 2017-03-24T14:58:19Z

@cloudRoutine agreed

cloudRoutine · 2017-03-25T14:36:56Z

@KevinRansom this'll be pretty useful when working on the parts of the compiler where there's extensive documentation in the signatures but barely any in the implementation files. So hopefully it can get merged soon.

cloudRoutine · 2017-03-28T01:09:51Z

@KevinRansom all green and resolved here too

cloudRoutine · 2017-03-28T19:26:30Z

integrated into #2683

cloudRoutine added 2 commits March 19, 2017 23:58

fix compilation with a few more pervasives

6276a97

sig and impl files both add doccoms to tooltips

a52c766

msftclas added the cla-already-signed label Mar 20, 2017

dsyme reviewed Mar 20, 2017

View reviewed changes

fix formatting

fe5ac92

cloudRoutine force-pushed the sig-doccoms branch from 3c269d4 to fe5ac92 Compare March 20, 2017 17:02

cloudRoutine added 2 commits March 20, 2017 15:45

fix build error

da3985f

Merge remote-tracking branch 'Microsoft/master' into sig-doccoms

1ec2071

Merge remote-tracking branch 'Microsoft/master' into sig-doccoms

32ca46f

cloudRoutine closed this Mar 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use doccoms from signature and implementation files for quickinfo tooltips #2662

Use doccoms from signature and implementation files for quickinfo tooltips #2662

cloudRoutine commented Mar 20, 2017

vasily-kirichenko commented Mar 20, 2017

cloudRoutine commented Mar 20, 2017

dsyme Mar 20, 2017

dsyme Mar 20, 2017

dsyme Mar 20, 2017

cloudRoutine Mar 20, 2017

dsyme Mar 20, 2017

dsyme Mar 20, 2017

dsyme Mar 20, 2017

dsyme commented Mar 20, 2017 •

edited

Loading

KevinRansom commented Mar 20, 2017

cloudRoutine commented Mar 20, 2017

cloudRoutine commented Mar 20, 2017

cloudRoutine commented Mar 22, 2017

cloudRoutine commented Mar 24, 2017

cartermp commented Mar 24, 2017

cloudRoutine commented Mar 25, 2017

cloudRoutine commented Mar 28, 2017

cloudRoutine commented Mar 28, 2017

Use doccoms from signature and implementation files for quickinfo tooltips #2662

Use doccoms from signature and implementation files for quickinfo tooltips #2662

Conversation

cloudRoutine commented Mar 20, 2017

vasily-kirichenko commented Mar 20, 2017

cloudRoutine commented Mar 20, 2017

dsyme Mar 20, 2017

Choose a reason for hiding this comment

dsyme Mar 20, 2017

Choose a reason for hiding this comment

dsyme Mar 20, 2017

Choose a reason for hiding this comment

cloudRoutine Mar 20, 2017

Choose a reason for hiding this comment

dsyme Mar 20, 2017

Choose a reason for hiding this comment

dsyme Mar 20, 2017

Choose a reason for hiding this comment

dsyme Mar 20, 2017

Choose a reason for hiding this comment

dsyme commented Mar 20, 2017 • edited Loading

KevinRansom commented Mar 20, 2017

cloudRoutine commented Mar 20, 2017

cloudRoutine commented Mar 20, 2017

cloudRoutine commented Mar 22, 2017

cloudRoutine commented Mar 24, 2017

cartermp commented Mar 24, 2017

cloudRoutine commented Mar 25, 2017

cloudRoutine commented Mar 28, 2017

cloudRoutine commented Mar 28, 2017

dsyme commented Mar 20, 2017 •

edited

Loading