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

Fix FormattingContext disposal #10887

Merged
merged 9 commits into from
Sep 17, 2024

Conversation

davidwengier
Copy link
Contributor

From a conversation on Teams. FormattingContext is disposable because it owns a workspace, but it also gets non-destructively mutated so it's unclear as to whether the disposal was working as intended, and unclear to consumers what needed to happen. Upon review of the code, this PR:

  • Moves workspace ownership to the caller of the formatting code, so its disposal is clear
  • Makes AdhocWorkspaceFactory shared code, because it did the same thing in OOP and LSP server
  • Adds IHostServicesProvider to OOP, because thats the thing that was actually different between OOP and LSP server
  • Random cleanup of some related things

@davidwengier davidwengier requested a review from a team as a code owner September 16, 2024 07:43
Copy link
Contributor

@alexgav alexgav left a comment

Choose a reason for hiding this comment

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

LGTM, my only thought is - do we still need the Provider if there is 1:1 mapping between them? Is there a lot of value in lazily creating the workspace via the provider?

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Thanks for the clean up! The disposal and ownership of the workspace is much cleaner, though I think the IDisposable contract in AdhocWorkspaceProvider can be tightened up a bit more.

@davidwengier
Copy link
Contributor Author

do we still need the Provider if there is 1:1 mapping between them?

Right now all of our tests pass if we don't use any of the host services provider stuff, and if VA Code formatting works then I don't know why we couldn't just have the code be new AdhocWorkspace() but I also don't know if I trust our current tests to actually make that determination, so I didn't scratch too far below the surface.

@davidwengier
Copy link
Contributor Author

Thank you for the excellent PR feedback! I have moved the workspace and document creation down even lower, so FormattingContext has nothing to do with it now. Much nicer IMO. Should have scratched the surface just a little bit more :)

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Very nice! I'm not sad to see IAdhocWorkspaceFactory removed. 😄

@davidwengier davidwengier merged commit 966d762 into dotnet:main Sep 17, 2024
12 checks passed
@davidwengier davidwengier deleted the AdhocWorkspaceChanges branch September 17, 2024 22:04
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Sep 17, 2024
@phil-allen-msft phil-allen-msft modified the milestones: Next, 17.12 P3 Oct 31, 2024
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.

4 participants