FSharp Shim - First Phase#34945
Conversation
|
Ok, I believe this is ready. // cc @heejaechang @tmat @jinujoseph How should we coordinate this? The easiest for me is to have a package on myget/etc as fast as possible so I can set the appropriate package version in the F# side of the PR. Also, which preview will this make it into for dev16.1? I ask because I hope we will have enough time to dogfood. // cc @ManishJayaswal @Pilchie I've tested the shim out myself and seems to be fine, but having time to dogfood would make us feel better. Technically, merging this would not break VisualFSharp immediately since we didn't remove IVTs yet, but the F# pr, dotnet/fsharp#6498, contains a fix for an API change from Roslyn. Also, Roslyn wants to modify the internal APIs I have already shimmed; that's why I want to make sure we get this coordinated. |
I think we should keep the current IVTs as they are, merge this PR to master which will produce a nuget package as soon as signed build is done, and once F# repo gets updated to consume this package we can remove the IVTs. |
| [Obsolete("Only used to allow IVTs to work temporarily, will be removed when IVTs are fully removed.")] | ||
| public static Microsoft.CodeAnalysis.Glyph Convert(FSharpGlyph glyph) | ||
| { | ||
| switch (glyph) |
There was a problem hiding this comment.
This block would be 1/3 the size by using a switch expression:
return glyph switch
{
FSharpGlyph.None => Microsoft.CodeAnalysis.Glyph.None,
FSharpGlyph.Assembly => Microsoft.CodeAnalysis.Glyph.Assembly,
...
};No real need to change it until such time as we have a refactoring in place to make it automatic. 😄
jinujoseph
added
the
Community
|
Integration test failure tracked as #35123 @jinujoseph for approval |
5 participants
This needs to be inserted in conjunction with this PR: dotnet/fsharp#6498