Skip to content
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

Merged
merged 10 commits into from
Apr 11, 2023

Conversation

MangelMaxime
Copy link
Contributor

@MangelMaxime MangelMaxime commented Apr 7, 2023

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 files DocumentationFormatter.fs, ParseAndCheckResults.fs, CommandResponse.fs, AdaptiveFSharpLspServer.fs, and FsAutoComplete.Lsp.fs.

🤖 Generated by Copilot at f78450b

The code for the tooltips was a mess
With tuples and arrays to compress
So they used EntityInfo
And TipFormatter to simplify
The logic and the data to express

📝🔧🧹

WHY

This PR improves various area around TipFormatter and Documentation, I tried to list all the changes that I made in the list below.

  • Support example tag inside of summary tag

Before
CleanShot 2023-04-07 at 15 00 48@2x

After
CleanShot 2023-04-07 at 14 59 27@2x


  • Display a link to open the documentation from the tooltip
CleanShot.2023-04-06.at.22.22.14.1.mp4

  • Fix info panel rendering when navigating via symbol inside of the user code

Before
image

After
CleanShot 2023-04-07 at 14 52 18@2x


  • Reduce logic duplication by moving more into TipFormatter
  • Use records instead of union for passing the data around
  • Truncate global examples from the tooltip
  • Change how returned type of fsharp/documentation instead of using a nested list it returns directly the data.

CleanShot 2023-04-07 at 15 04 06@2x

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

  • Introduce new types EntityInfo and TryGetToolTipEnhancedResult to represent the information about the symbols and the tooltips in a more structured and descriptive way (link, link, link)
  • Refactor the 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)
  • Refactor the getTooltipDetailsFromSymbolUse function and its callers to use the EntityInfo type instead of a tuple of six arrays, and make the code more readable and consistent (link, link, link, link, link, link, link)
  • Refactor the formattedDocumentation and formattedDocumentationForSymbol functions and their callers to use the DocumentationDescription 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)
  • Replace the use of emptyTypeTip, which was a tuple of six empty arrays, with EntityInfo.Empty, which is a record of the same shape, in the DocumentationFormatter module (link, link, link, link, link)
  • Use the 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 the AdaptiveFSharpLspServer and FsAutoComplete.Lsp modules (link, link)
  • Add an open statement for the FSharp.Compiler.Symbols namespace in the CommandResponse module, which is needed to use the FSharpXmlDoc type (link)
  • Rename the Types field to DeclaredTypes and the Footer field to FooterLines in the DocumentationDescription type, to avoid confusion and indicate the data structure (link)

Maxime Mangel added 3 commits April 7, 2023 14:36
- 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
@MangelMaxime
Copy link
Contributor Author

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 🤣

@MangelMaxime MangelMaxime force-pushed the feature/improve_tooltips branch from eddf45f to e613e7a Compare April 7, 2023 15:28
@MangelMaxime
Copy link
Contributor Author

Yes ! Finally, the tests are green

Copy link
Contributor

@baronfel baronfel left a 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.

Comment on lines +19 to +34
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 = [||] }
Copy link
Contributor

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!

Copy link
Contributor Author

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.

src/FsAutoComplete.Core/TipFormatter.fs Outdated Show resolved Hide resolved

let text =
if hasTruncatedExamples then
"Open the documentation to see the truncated examples"
Copy link
Contributor

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:

Suggested change
"Open the documentation to see the truncated examples"
"Open the documentation to see more examples"

Copy link
Contributor Author

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 <baronfel@users.noreply.github.com>
@baronfel
Copy link
Contributor

Lovely work, I'm going to merge this and work on the matching Ionide release.

@baronfel baronfel merged commit 37648cb into ionide:main Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants