search

fsprojects / FSharp.Formatting

F# tools for generating documentation (Markdown processor and F# code formatter)
https://fsprojects.github.io/FSharp.Formatting/
Other
462 stars 155 forks source link

Function signature tooltips include attributes for arguments that make the tooltip unreadable #858

Open kMutagene opened 8 months ago

kMutagene commented 8 months ago

I am not sure if this is the correct place to raise an issue like this, because it also affects both tooltips in VS and polyglot notebooks

Plotly.NET makes quite heavy use of attributes for method arguments to improve C# interop. A typical method looks like this:

[<Extension>]
static member Point
    (
        x: seq<#IConvertible>,
        y: seq<#IConvertible>,
        [<Optional; DefaultParameterValue(null)>] ?Name: string,
        [<Optional; DefaultParameterValue(null)>] ?ShowLegend: bool,
        [<Optional; DefaultParameterValue(null)>] ?Opacity: float,
        [<Optional; DefaultParameterValue(null)>] ?MultiOpacity: seq<float>,
        [<Optional; DefaultParameterValue(null)>] ?Text: #IConvertible,

       <many more arguments here>

    ) = <body here>

in the past, tooltips for these methods looked like this:

image

with the latest fsdocs version, they include the Optional and DefaultParameterValue Attributes, even prefixed with their full namespaces:

image

This is really unfortunate because they are now completely unusable. This also happens in Ionide and Visual studio:

image

image

So i suspect something bigger out of the scope of this lib being at play. Might this be related to this lib updating the FCS version with > 19.0.0? Is there another place to raise this issue? This is impacting the usability of Plotly.NET heavily.

nojaf commented 8 months ago

This rings a bell. I believe this is a change with NicePrint over in the F# compiler. I remember some time ago that I wanted to include parameter attributes in the signatures files. This impacts:

image

I don't think this is problematic in itself. It accurately reflects what the code contains. The formatting maybe could improve over at the compiler side? I'm not sure, maybe it is fine and what the ID does with the result should improve. I also think Ionide does its own thing with tooltips.

And I'm pretty sure that fsharp-formatting might also drop the ball in the tooltip department.

kMutagene commented 8 months ago

I don't think this is problematic in itself. It accurately reflects what the code contains.

While technically correct, i am not sure if i agree here generally. Maybe my use case for these tooltips is non-standard, but i generally want to know the names and types of the arguments - which are really hidden in visual noise, even when formatted better like in your screenshot. It also might be that the methods of Plotly.NET just have an unusual amount of arguments (sometimes > 100), so it might be an extreme case where tooltips reach their limit anyways.

I do not want to sound too complain-y here though - at the end of the day if this the way to display tooltips i'll adapt, but i think for this specific use case the update made things worse.

If you could point me to the code where tooltips are generated, i could add a flag to the tool that trims these from the input if that is a possibility. Should be quite easy to remove with a regex, as everything is guarded by [<>]

As an aside - the API docs tooltips for methods look 100x better than this IMO. could we just display that as tooltip (without the param descriptions)?

image

nojaf commented 8 months ago

I think extending the DisplayEnvironment (in https://github.com/dotnet/fsharp/blob/1e78d396a1397009fe52d609660376f0ce5c5d52/src/Compiler/Symbols/SymbolHelpers.fs#L546-L551) and only add the attributes when our new flag is enabled (Somewhere in https://github.com/dotnet/fsharp/blob/4934588ae8dfac3de2e8ce58198601edbfddfd08/src/Compiler/Checking/NicePrint.fs#L1047) makes the most sense. We would disable it for tooltips.

We can pair on this sometime if you want.

kMutagene commented 8 months ago

We can pair on this sometime if you want.

Sure, do you intend to stream this? I am not sure if i will have time in the next week(s), but i'll make sure to reach out once i am free

nojaf commented 8 months ago

Sure, do you intend to stream this?

We could but that is really up to you. I wasn't specifically proposing it with that in mind.