Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes for inlay hint #73

Merged
merged 3 commits into from
Mar 4, 2023
Merged

Conversation

tcx4c70
Copy link
Contributor

@tcx4c70 tcx4c70 commented Mar 3, 2023

No description provided.

tcx4c70 added 2 commits March 3, 2023 23:50
According to microsoft doc [1], `ISymbol.Name` property might be a empty
string, which leads to empty inlay hints for some types, e.g., list of
strings. Replacing it with `ISymbol.ToDisplayParts` to avoid empty inlay
hint labels.

[1] https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.isymbol.name?view=roslyn-dotnet-4.3.0#microsoft-codeanalysis-isymbol-name

Fixes: 1f3c857 ("feat(Server.fs): Support type inlay hint")
Signed-off-by: Adam Tao <tcx4c70@gmail.com>
Signed-off-by: Adam Tao <tcx4c70@gmail.com>
@tcx4c70
Copy link
Contributor Author

tcx4c70 commented Mar 3, 2023

Because of the GFW of China, some screenshots can't be uploaded :)

I was testing with the following example code:

class Program
{
    static void Main()
    {
        var list = new [] { "a", "b" };
        var (x, y) = (0, "abc");
    }
}

Signed-off-by: Adam Tao <tcx4c70@gmail.com>
@tcx4c70 tcx4c70 changed the title 2 fixes for inlay hint Fixes for inlay hint Mar 3, 2023
@razzmatazz razzmatazz merged commit ef43840 into razzmatazz:master Mar 4, 2023
@razzmatazz
Copy link
Owner

LGTM!

@tcx4c70 tcx4c70 deleted the fix/inlay-hint branch March 4, 2023 09:29
@razzmatazz
Copy link
Owner

@tcx4c70 I was wondering if it is possible to somehow remove typehints for assignments where right side already has type name mentioned, e.g. now we get this:

Bildschirm­foto 2023-03-18 um 16 05 32

so I was thinking if we can do some heuristics that:

  • (a) avoid type hints for "var v = new XXX()" assignments
  • (b) avoid type hints for "var v = expression()" assignments

from looking at both cases above, maybe we can generalize and avoid type hints if expression on the right side already contains type name?

sorry -- maybe I should create a new issue here, but still

@razzmatazz
Copy link
Owner

I can take a look and change your code regarding inlay hints, if you agree that this warrants fixing

@razzmatazz
Copy link
Owner

razzmatazz commented Mar 18, 2023

we are also missing test infrastructure, which could help us greatly :(

@tcx4c70
Copy link
Contributor Author

tcx4c70 commented Apr 22, 2023

@tcx4c70 I was wondering if it is possible to somehow remove typehints for assignments where right side already has type name mentioned, e.g. now we get this:

Bildschirm­foto 2023-03-18 um 16 05 32

so I was thinking if we can do some heuristics that:

  • (a) avoid type hints for "var v = new XXX()" assignments
  • (b) avoid type hints for "var v = expression()" assignments

from looking at both cases above, maybe we can generalize and avoid type hints if expression on the right side already contains type name?

@razzmatazz Sorry that I missed your reply. I also notice the duplicate type hints you mentioned, but i was busy on my work for last month and have no time to improve it. Maybe I'll investigate and imrove it on the next holiday on May.

@tcx4c70
Copy link
Contributor Author

tcx4c70 commented Apr 22, 2023

we are also missing test infrastructure, which could help us greatly :(

Yes, I struggled on testing during my development. Maybe we can add test infrastructure to roadmap or TODO list?

@razzmatazz
Copy link
Owner

razzmatazz commented Apr 22, 2023

we are also missing test infrastructure, which could help us greatly :(

Yes, I struggled on testing during my development. Maybe we can add test infrastructure to roadmap or TODO list?

I was pondering if we can steal that (or most of the bits) from ionide.fsautocomplete

lately i'm a little bit too busy but it should be possible to produce something working by copying the files in and doing some rough hacking

@razzmatazz
Copy link
Owner

razzmatazz commented Apr 22, 2023

i'm also not sure how they (fsautocomplete) do the testing, at which level, if in-proc or out of process --the later feels better for me

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants