Skip to content
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

[VS] Consolidate Roslyn workspace and FCS #11694

Merged
merged 28 commits into from
Jun 24, 2021
Merged

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Jun 18, 2021

This is a refactoring PR for the most part.

I've been testing out the in-memory documents PR and it's a little unstable. We need the ability to unit test VS behavior reliably.
Currently, our VS tests do not even use FSharpProjectOptionsManager which I think it should as it plays an large role for VS (in the future, we may be able to get rid of it, but not now).

So, this PR is to consolidate Rolsyn workspaces and FCS into one place called WorkspaceExtensions.

@TIHan TIHan changed the title [WIP][VS] Consolidate Roslyn workspace and FCS [VS] Consolidate Roslyn workspace and FCS Jun 22, 2021
@TIHan
Copy link
Contributor Author

TIHan commented Jun 22, 2021

@KevinRansom @dsyme This is ready when the tests pass.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 23, 2021

Wonderful, it's passing :)

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

This is great. The one thing is I'd like us please not to lose the userOpName strings - propagate them through.


items.Add dataItem

d.ForEach(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Stylistically I'd prefer d.ForEach(fun dataItem -> ...) rather than using a local function. But that's unrelated to this PR

@TIHan
Copy link
Contributor Author

TIHan commented Jun 23, 2021

@dsyme this is ready again.

) =
inherit CodeFixProvider()
let fixableDiagnosticIds = ["FS0053"]
static let userOpName = "ProposeUpperCaseLabel"
Copy link
Contributor

Choose a reason for hiding this comment

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

Still some userOpName being dropped but I'll approve this as is

asyncMaybe {
let! sourceText = document.GetTextAsync() |> liftTaskAsync
let filePath = document.FilePath
let! symbol = document.TryFindFSharpLexerSymbolAsync(position, SymbolLookupKind.Greedy, false, false, nameof(FSharpDocumentHighlightsService.GetDocumentHighlights))
Copy link
Contributor

Choose a reason for hiding this comment

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

Stylistically I think I prefer binding userOpName at the top of the scope like before but it's no big deal.

@dsyme dsyme merged commit 48e06db into dotnet:main Jun 24, 2021
@TIHan TIHan mentioned this pull request Aug 16, 2021
10 tasks
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
[VS] Consolidate Roslyn workspace and FCS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants