F# HoverText #714
F# HoverText #714
Conversation
|
|
|
This looks wonderful! Can you add the appropriate tests to the Long term we'd likely want this code migrated to the |
src/FsAutoComplete/Debug.fs
Outdated
| @@ -0,0 +1,39 @@ | |||
| // -------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
It looks like this file can be removed.
|
An interesting enhancement here to traditional hover text is that we can also inspect the current value of the variable using |
|
I'll add some basic tests and remove more unnecessary code. @jonsequitur awesome idea, I will definetly look into that... |
|
Hey, I just added a few tests and improved the layouting a little. Btw. I also included the current-value when available: |
|
That is very cool! |
|
This is great work and I think we will be able to take this after an iteration or too
We do need to be consolidating here. Referencing ionide/FsAutoComplete#645 which covers the ground |
|
@krauthaufen Did you consider calling the FCS method These tooltips are the ones used in Visual Studio, which are not too bad though Ionide has its own point of view on the content to show |
| newFooter | ||
| ] | ||
|
|
||
| FormattedValue("text/markdown", stdinRx.Replace(fsiModuleRx.Replace(markdown, ""), "")) |
There was a problem hiding this comment.
Why are we producing text/markdown? Does HoverTextProduced require it?
There was a problem hiding this comment.
It's basically what ionide does and the tooltip-text contains markdown for e.g. generic arguments. Furthermore the F# syntax highlighting works with markdown code-blocks...
There was a problem hiding this comment.
Furthermore the F# syntax highlighting works with markdown code-blocks...
Right, though that's a non-standard interpretation of F# code comments only really supported by F# tooling based off the ionide toolchain.
We're talking about standardizing it but it's a long way off, I don't think we should adopt it in F# notebooks
| return None | ||
| } | ||
|
|
||
| match! res.TryGetToolTipEnhanced (FSharp.Compiler.Range.mkPos line col) lineContent with |
There was a problem hiding this comment.
Yes I'm wondering why we need FsAutoComplete tooltips here, why not the tooltips provided by FCS. That might greatly reduce the amount of code copied across (and leave us to possibly incorporate the FsAutoComplete tooltips into FCS later).
There was a problem hiding this comment.
I first tried to use the plain tooltips from FCS but they don't incorporate types/modules etc. (at least when used with overlapping names).
When hovering the type int i got the tooltip for the int : ^T -> int function, so i figured FsAutoComplete could be a way to go.
There was a problem hiding this comment.
I first tried to use the plain tooltips from FCS but they don't incorporate types/modules etc. (at least when used with overlapping names).
You're right that the FsAutoComplete tooltips make improvements (though it's not yet clear to me if they also regress other things).
But in any case, for .NET Interactive we would take the approach that we build on FSharp.Compiler.Service and just have to accept the tooling possibilities that gives. If we need to improve those tooltips, let's do it in FCS (e.g. by incorporating and aligning the tooltips code with Ionide)
| match fullName with | ||
| | Some name -> | ||
| let expr = if name.StartsWith "Stdin." then name.Substring 6 else name | ||
| try return script.Value.Fsi.EvalExpression(expr) |> Some |
There was a problem hiding this comment.
I'd like to understand more about how "safe" this evaluation is (i.e. not have side-effects)
There was a problem hiding this comment.
Since it just runs for module-bindings I figured they were already evaluated in the FSI-semantics?
|
Note to self: the static checking in this PR (from which the tooltips are driven) appears to be based on the current type-check state of FSI, plus the checking of the cell contents of the cell being examined (but not of prior cell contents). This differs from iFSharp etc. where the static checking concatenates all cell contents and then splits them out again. This means executing cells (especially definitions and package loads) will light up more accurate tooling in subsequent cells |
We've considered the possibility of doing a compile for language services evaluation when the notebook starts up. This would make the assumption that the cells should be evaluated in order, which I think is intuitive. It would present some challenges for polyglot scenarios where a submission in one languages influences the state of another, e.g. variable sharing. |
|
@krauthaufen Can you update each test in the file Once that file's updated (and a merge or rebase to fix conflicts) I'm ready to pull it. |
# Conflicts: # dotnet-interactive.sln
|
Hey, sorry for the delay, I addressed all your requests however I have no idea how useful these tests are since they're very strict in the Hover-Texts. They all succeed atm. but minor format changes would immediately break them. |
|
Thanks for adding those F# tests. Yes, they're very strict, but they test that the F# kernel is setup to receive those commands, which is subtly different than directly testing the kernel itself. The CI failure appears to be a refactoring that's causing problems with your merge from |
|
@krauthaufen Thank you again for porting over F# hover text. I was eager to use it so I've published a new version of the VS Code extension. Version 1.0.146603 has F# hover text and it's beautiful. |
|
Cool, thanks |
Hey interactive people, I added support for
RequestHoverTextinFSharpKernelvia adding (a very reduced variant) of FsAutoComplete.I'm aware that this is not really pretty, since changes in
FsAutoCompletewon't be easy to integrate (some files are modified slightly) but nonetheless hover-texts are really useful (escpecially for F#).Maybe you have a better idea how to integrate FsAutoComplete's hover-text.
Cheers