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

Language service tidying #9605

Merged
merged 11 commits into from
Nov 28, 2024
Merged

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Nov 28, 2024

I'm familiarising myself more deeply with the language service code, ahead of fixing a bug in it.

As part of that familiarisation, I updated the code to use some newer C# features:

  • File-scoped namespaces
  • Primary constructors
  • Local functions

To me, this makes the code a bit easier to work with.

I also removed some unused parameters from interfaces and their interfaces. No public changes.

There are no functional changes here. Just reformatting code and removing unused stuff. I wanted to separate this out from the bug fix, which will come in a later PR.

There are quite a few whitespace changes here, which makes the review a bit harder. Might be easiest to disable whitespace in the diff, and go commit-by-commit.

Microsoft Reviewers: Open in CodeFlow

@drewnoakes drewnoakes added the Feature-Language-Service Populating the Roslyn workspace with references, source files, analyzers, etc label Nov 28, 2024
@drewnoakes drewnoakes requested a review from a team as a code owner November 28, 2024 02:00
{
value.Value(context.BinOutputPath, added, removed);
value(context.BinOutputPath, added, removed);
Copy link
Contributor

@zewditu zewditu Nov 28, 2024

Choose a reason for hiding this comment

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

This question is for my own learning.
Why you need this change? is that to gain code cleanness or performance gain?

Copy link
Member Author

Choose a reason for hiding this comment

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

ExtensionValues makes the code here a little cleaner, and it also ensures that any MEF exports that might throw when instantiated are filtered out, rather than causing this code to crash.

I guess when I said there was no behaviour change, I should have called this out.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -52,6 +53,7 @@ proj
projectsystem
projitems
readonly
refactorings
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need them for future use? or are they used somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This word was used in the code somewhere. While it's not actually a real word in the dictionary, it's common enough in code that I don't think it should be marked as a spelling error.

{
value.Value(context.BinOutputPath, added, removed);
}
value(context.BinOutputPath, added, removed);
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should come to here, it is outdated in original place

Copy link
Member Author

Choose a reason for hiding this comment

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

I answered above.

@drewnoakes drewnoakes merged commit fc09b4a into dotnet:main Nov 28, 2024
5 checks passed
@drewnoakes drewnoakes deleted the lang-svc-modernisation branch November 28, 2024 11:12
@dotnet-policy-service dotnet-policy-service bot added this to the 17.12 milestone Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Language-Service Populating the Roslyn workspace with references, source files, analyzers, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants