-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various improvement around TipFormatter and Documentation #1099
Conversation
- Use records instead of union for passing the data around - Truncated global example from the tooltip - Display a link to open the documentation from the tooltip - Reduce logic duplication by moving more into TipFormatter - Fix info panel rendering when navigating via symbol inside of the user code
Seems like I need to learn how to fix the tests for this PR. And, I guess we will not need to write changelog in the PR in the future, next step is for Copilot to generate my pictures report ahah 🤣 |
This mimic the old behaviours and should fix the remaining tests
4c28976
to
f9cce33
Compare
eddf45f
to
e613e7a
Compare
Yes ! Finally, the tests are green |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are a magician, my friend.
type EntityInfo = | ||
{ Constructors: string array | ||
Fields: string array | ||
Functions: string array | ||
Interfaces: string array | ||
Attributes: string array | ||
DeclaredTypes: string array } | ||
|
||
static member Empty = | ||
|
||
{ Constructors = [||] | ||
Fields = [||] | ||
Functions = [||] | ||
Interfaces = [||] | ||
Attributes = [||] | ||
DeclaredTypes = [||] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change alone would be a wonderful PR - thank you for taking the time to make this nicer!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ah, yes having a record instead of the tuple make it so much easier to understand what you have in hand.
I was getting lost with stuff like string * (string * string []) * (string * string * string * string * string * string)
etc.
|
||
let text = | ||
if hasTruncatedExamples then | ||
"Open the documentation to see the truncated examples" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does 'truncated' here mean that there were so many examples that we didn't show them all? If so, what about the following message:
"Open the documentation to see the truncated examples" | |
"Open the documentation to see more examples" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
truncated
means that there was at least 1 example node at the root of the doc comment.
In the following snippet the example is going to be truncated from the tooltip result because it is a direct child of the root.
<summary>
Create a promise from a function
</summary>
<typeparam name="'T">Return type of the promise</typeparam>
<returns>
The promise created by the function
</returns>
<example>
<code lang="fsharp">
let write (path: string) (content: string) =
Promise.create (fun resolve reject ->
Node.Api.fs.writeFile(path, content, (fun res ->
match res with
| Some res -> reject (res :?> System.Exception)
| None -> resolve ()
))
)
</code>
</example>
In the following snippet, the example is not truncated because it is part of another node (here the summary
node).
<summary>
Create a promise from a function
<example>
<code lang="fsharp">
let write (path: string) (content: string) =
Promise.create (fun resolve reject ->
Node.Api.fs.writeFile(path, content, (fun res ->
match res with
| Some res -> reject (res :?> System.Exception)
| None -> resolve ()
))
)
</code>
</example>
</summary>
<typeparam name="'T">Return type of the promise</typeparam>
<returns>
The promise created by the function
</returns>
We makes the distinction here, because if we truncate the examples when inside another nodes we are loosing context and depending on how the text was formulated it could makes the tooltip not useful.
Same, if there are examples in different nodes without that, we would just concatenate them at the end of the tooltip but we would not know if they were in the summary, exceptions, returns, etc. nodes.
Co-authored-by: Chet Husk <[email protected]>
Lovely work, I'm going to merge this and work on the matching Ionide release. |
WHAT
🤖 Generated by Copilot at f78450b
This pull request refactors the code for tooltip and documentation formatting across different modules and servers. It introduces new types and functions to improve the readability, consistency, and structure of the code and the data types. It also uses the
TipFormatter
module to centralize the formatting logic. It affects the filesDocumentationFormatter.fs
,ParseAndCheckResults.fs
,CommandResponse.fs
,AdaptiveFSharpLspServer.fs
, andFsAutoComplete.Lsp.fs
.🤖 Generated by Copilot at f78450b
📝🔧🧹
WHY
This PR improves various area around TipFormatter and Documentation, I tried to list all the changes that I made in the list below.
Before
After
CleanShot.2023-04-06.at.22.22.14.1.mp4
Before
After
fsharp/documentation
instead of using a nested list it returns directly the data.Note: According to the previous implementation in FsAutoComplete the first list was always having a single element otherwise it was being thrown away.
HOW
🤖 Generated by Copilot at f78450b
EntityInfo
andTryGetToolTipEnhancedResult
to represent the information about the symbols and the tooltips in a more structured and descriptive way (link, link, link)TryGetToolTipEnhanced
function and its callers to use the new types instead of tuples and options, and handle the cases where the formatting of the tooltip text fails or returns none (link, link, link, link, link, link, link)getTooltipDetailsFromSymbolUse
function and its callers to use theEntityInfo
type instead of a tuple of six arrays, and make the code more readable and consistent (link, link, link, link, link, link, link)formattedDocumentation
andformattedDocumentationForSymbol
functions and their callers to use theDocumentationDescription
type instead of a tuple of lists and strings, and handle the cases where the formatting of the documentation comment fails or returns none (link, link, link, link, link, link)emptyTypeTip
, which was a tuple of six empty arrays, withEntityInfo.Empty
, which is a record of the same shape, in theDocumentationFormatter
module (link, link, link, link, link)TipFormatter
module to prepare the signature and the footer lines, and to render a link to show the documentation if the tooltip has truncated examples, in theAdaptiveFSharpLspServer
andFsAutoComplete.Lsp
modules (link, link)FSharp.Compiler.Symbols
namespace in theCommandResponse
module, which is needed to use theFSharpXmlDoc
type (link)Types
field toDeclaredTypes
and theFooter
field toFooterLines
in theDocumentationDescription
type, to avoid confusion and indicate the data structure (link)