-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
F# Shim - Round 2 #35591
F# Shim - Round 2 #35591
Conversation
This is basically done until some review. However, I'm having some trouble getting DocumentDiagnosticAnalyzers to run. None of them seem to work since I moved them over to the ExternalAccess.FSharp. But, with the services that I have tested at this point, inline rename, block structure, navigation, those seem to work. |
This is ready. @sharwell helped me get the DocumentDiagnosticAnalyzers working again. I've tested all the services shimmed; they seem to work. Once we get this into preview we can dogfood a lot more. |
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.
nice!
nice that those IVT are now removed except VS one for step 3. |
} | ||
} | ||
|
||
internal static FSharpGlyph GetExpectedFSharpGlyph(Microsoft.CodeAnalysis.Glyph glyph) |
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.
💡 Since this is a test, can you make it use reflection?
} | ||
} | ||
|
||
internal static Microsoft.CodeAnalysis.Glyph GetExpectedGlyph(FSharpGlyph glyph) |
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.
💡 Since this is test code, can you make it use reflection?
} | ||
} | ||
|
||
internal static FSharpSignatureHelpTriggerReason GetExpectedTriggerReason(SignatureHelpTriggerReason triggerReason) |
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.
💡 Since this is a test, can it use reflection on the member name?
} | ||
} | ||
|
||
internal static NavigateToMatchKind GetExpectedNavigateToMatchKind(FSharpNavigateToMatchKind kind) |
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.
💡 Since this is test code, can it use reflection on the member name?
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.
We could, but I don't know if that's really what we want. What if we have a mapping between enum cases where the name doesn't match?
src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs
Outdated
Show resolved
Hide resolved
src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs
Outdated
Show resolved
Hide resolved
src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs
Outdated
Show resolved
Hide resolved
src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs
Outdated
Show resolved
Hide resolved
src/Tools/ExternalAccess/FSharp/Internal/Editor/FSharpEditorInlineRenameService.cs
Outdated
Show resolved
Hide resolved
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.
looks good!
approved for 16.2.preview4 |
* For background, see: * dotnet#35591 * dotnet/fsharp#16078, * dotnet/fsharp#16079 (comment) * dotnet/fsharp#16079 (review) * dotnet/fsharp#16211
This will actually remove F#'s use of Roslyn IVTs, except for
Microsoft.VisualStudio.LanguageServices
.I've added a new project,
Microsoft.CodeAnalysis.ExternalAccess.FSharp.UnitTests
, in aFSharpTest
folder next toFSharp
in theExternalAccess
folder. Currently, it is just testing enums that were shimmed over. I will add a test for the Glyph enum which is related to this issue: #34971