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

Move code off of HostLanguageServices in favor of HostProjectServices. #62949

Merged
merged 16 commits into from
Jul 27, 2022

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Jul 26, 2022

Followup to #62947.

@CyrusNajmabadi CyrusNajmabadi requested a review from tmat July 26, 2022 19:19
@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners July 26, 2022 19:20
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

As much as I am saddened by having all the fields/properties still called language services...I'm not sure I want to review that diff.

@@ -218,7 +218,9 @@ public static SyntaxEditor GetSyntaxEditor(this Document document, SyntaxNode ro
{
#if CODE_STYLE
// Remove once Solution.Services becomes public: https://github.com/dotnet/roslyn/issues/62914
#pragma warning disable RS0030 // Do not used banned APIs. This is the shim method to use until the api becomes public.
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth clarifying this comment a bit more -- not entirely sure what you mean by "the API' here.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. can do.

@@ -151,12 +151,12 @@ private static bool MatchesCommentStart(string commentStart, ITextSnapshotLine l
ITextBuffer textBuffer,
SnapshotSpan selectionSpan)
{
var syntaxKinds = document.LanguageServices.GetRequiredService<ISyntaxKindsService>();
var syntaxKinds = document.ProjectServices.GetRequiredService<ISyntaxKindsService>();
Copy link
Member

Choose a reason for hiding this comment

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

We have both document.ProjectServices and document.Project.Services? Do we really need both? Is there a shortage of .s these days?

Copy link
Member Author

Choose a reason for hiding this comment

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

so this is not a normal document. this is the internal ParsedDocument helper guy. So this is how that guy bridges to this new api.

@@ -99,7 +99,7 @@ public CompletionRules GetRules()
OptionSet? options = null)
{
var document = text.GetOpenDocumentInCurrentContextWithChanges();
var languageServices = document?.Project.LanguageServices ?? _services.GetLanguageServices(Language);
var languageServices = document?.Project.Services ?? _services.GetLanguageServices(Language).ProjectServices;
Copy link
Member

Choose a reason for hiding this comment

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

Renaming the local or no?

Copy link
Member Author

Choose a reason for hiding this comment

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

generally no. because it's just going to make these even large. and, frankly, 'language services' isn't a name that bothers me. it still is the thing you ask for for ILanguageService's.

@@ -22,7 +22,7 @@ internal static class CodeActionOptionsStorage
public static readonly PerLanguageOption2<int> WrappingColumn =
new("FormattingOptions", "WrappingColumn", CodeActionOptions.DefaultWrappingColumn);

public static CodeActionOptions GetCodeActionOptions(this IGlobalOptionService globalOptions, HostLanguageServices languageServices)
public static CodeActionOptions GetCodeActionOptions(this IGlobalOptionService globalOptions, HostProjectServices languageServices)
Copy link
Member

Choose a reason for hiding this comment

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

Rename the parameter?

@@ -302,7 +302,7 @@ private async Task ApplySuppressionFixAsync(IEnumerable<DiagnosticData>? diagnos

// We have different suppression fixers for every language.
// So we need to group diagnostics by the containing project language and apply fixes separately.
var languageServices = new HashSet<HostLanguageServices>(projectDiagnosticsToFixMap.Select(p => p.Key.LanguageServices).Concat(documentDiagnosticsToFixMap.Select(kvp => kvp.Key.Project.LanguageServices)));
var languageServices = projectDiagnosticsToFixMap.Select(p => p.Key.Services).Concat(documentDiagnosticsToFixMap.Select(kvp => kvp.Key.Project.Services)).ToHashSet();
Copy link
Member

Choose a reason for hiding this comment

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

Should this just be using language name here? This is really funky we're using the language service type this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

no disagreement. but would like to keep mechanical for now.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge July 26, 2022 22:59
@CyrusNajmabadi CyrusNajmabadi merged commit bfb1e25 into dotnet:main Jul 27, 2022
@ghost ghost added this to the Next milestone Jul 27, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the banAPI3 branch July 27, 2022 17:08
@dibarbet dibarbet removed this from the Next milestone Sep 1, 2022
@dibarbet dibarbet added this to the 17.4 P2 milestone Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants