-
Notifications
You must be signed in to change notification settings - Fork 215
Rationalize ProjectEngineHost a bit #11685
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
Rationalize ProjectEngineHost a bit #11685
Conversation
It seems this is unused.
- Nest ProjectEngineFactory inside ProjeectEngineFactories, make it private, and rename it to SimpleFactory. - Mark ProjectEngineFactoryProvider as sealed - Mark DefaultProjectEngineFactory as sealed
The is no longer used.
There's a surprising amount of production code checking IsUnsupported() and specific tests calling SetUnsuported() that can be removed.
After calling DocumentContext.GetCodeDocumentAsync(...), it's unnecessary to call DocumentContext.GetSourceTextAync(...). Instead, just use RazorCodeDocument.Source.Text.
} | ||
|
||
var codeDocument = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false); | ||
if (codeDocument.IsUnsupported()) |
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.
I'm so happy to see this go 🥳
} | ||
|
||
var codeDocument = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false); | ||
var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false); |
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.
not for this PR, but should DocumentContext.GetCodeDocumentAsync
populate _sourceText
with this value as well? I assume we have the two separated for some optimization where you need sourcetext but not a full RazorCodeDocument
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.
Yes, that's a very good idea.
Thanks for making this easy to review! |
This is a follow up to #11675 that cleans up and removes some code in the "ProjectEngineHost" folder. Notably, it turns out that the system for marking projects as "unsupported" is no longer used. So, checks for
RazorCodeDocument.IsUnsupported()
can be removed from the language server and tests for this scenario can be removed.NOTE: This PR is currently based onpre-snap-code-clean-up
to aid code reviews. I'll rebase it tomain
once #11675 is merged.Changed the base to
main