Skip to content

F# Shim - Round 2#35591

Merged
TIHan merged 25 commits into
dotnet:masterfrom
TIHan:fsharp-shim-v2
Jun 11, 2019
Merged

F# Shim - Round 2#35591
TIHan merged 25 commits into
dotnet:masterfrom
TIHan:fsharp-shim-v2

Conversation

@TIHan

@TIHan TIHan commented May 8, 2019

Copy link
Copy Markdown
Contributor

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 a FSharpTest folder next to FSharp in the ExternalAccess 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

@TIHan TIHan requested review from a team as code owners May 8, 2019 19:57
@TIHan TIHan changed the title [WIP] F# Shim - Round 2 F# Shim - Round 2 May 26, 2019
@TIHan

TIHan commented May 26, 2019

Copy link
Copy Markdown
Contributor Author

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.

@TIHan

TIHan commented May 28, 2019

Copy link
Copy Markdown
Contributor Author

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.

@heejaechang heejaechang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@heejaechang

Copy link
Copy Markdown
Contributor

nice that those IVT are now removed except VS one for step 3.
there is no public API added in FSharp Access and no change in Roslyn side.
changes in ExternalAccess is up to you guys, so all looks good. I like that you added unit test as well.

Comment thread src/Tools/ExternalAccess/FSharp/ExternalAccessFSharpResources.resx
}
}

internal static FSharpGlyph GetExpectedFSharpGlyph(Microsoft.CodeAnalysis.Glyph glyph)

Copy link
Copy Markdown
Contributor

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)

Copy link
Copy Markdown
Contributor

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)

Copy link
Copy Markdown
Contributor

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)

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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?

Comment thread src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs Outdated
Comment thread src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs Outdated
Comment thread src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs Outdated
Comment thread src/Tools/ExternalAccess/FSharp/Editor/IFSharpEditorInlineRenameService.cs Outdated

@heejaechang heejaechang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@TIHan TIHan closed this Jun 10, 2019
@TIHan TIHan reopened this Jun 10, 2019
@TIHan TIHan closed this Jun 11, 2019
@TIHan TIHan reopened this Jun 11, 2019
@jinujoseph jinujoseph added this to the 16.2 milestone Jun 11, 2019
@jinujoseph

Copy link
Copy Markdown
Contributor

approved for 16.2.preview4

@TIHan TIHan merged commit 0d44ad1 into dotnet:master Jun 11, 2019
brianrourkeboll added a commit to brianrourkeboll/roslyn that referenced this pull request Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants