Skip to content

Conversation

@allisonchou
Copy link
Contributor

@allisonchou allisonchou commented Jun 15, 2023

This PR bumps our Roslyn version to take a logging fix for integration tests (dotnet/roslyn#68619). Bumping the Roslyn version resulted in us also needing to bump our LanguageServer.Protocol version, which introduces a new breaking type (InsertReplaceEdit). We should handle this type in a follow-up PR, however it doesn't seem to be urgent since neither the VS or VS Code clients support it yet.

@allisonchou allisonchou changed the title Try bump Roslyn version Bump Roslyn version for integration test fix Jun 19, 2023
@allisonchou allisonchou marked this pull request as ready for review June 19, 2023 05:08
@allisonchou allisonchou requested review from a team as code owners June 19, 2023 05:08
var vsCompletionList = Assert.IsType<OptimizedVSCompletionList>(result.Value.Value);
var item = vsCompletionList.Items.First();
Assert.Equal("DateTime", item.Label);
var item = vsCompletionList.Items.Where(item => item.Label == "DateTime");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why the ordering of C# completion items changed, but it doesn't affect the end result in practice since the client seems to sort the items afterwards

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the problem that we're getting an unfiltered completion list instead of one filtered to DateTime?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it is ordering things differently. I bet it's just a change in Roslyn.

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.

We shouldn't be reverting the semantic token work. The Protocol bump is all that's needed to fix that.

@allisonchou
Copy link
Contributor Author

allisonchou commented Jun 20, 2023

@DustinCampbell just pinged you offline, but the Protocol bump was already present in this PR beforehand and even with it, we were still seeing issues with hanging CI. We might need to still revert the semantic tokens work for now

@DustinCampbell DustinCampbell dismissed their stale review June 20, 2023 22:08

The plan is to use this PR, but we're going to get it cleaned up first.

@davidwengier davidwengier enabled auto-merge June 20, 2023 23:43
@davidwengier davidwengier merged commit f7483d3 into main Jun 21, 2023
@davidwengier davidwengier deleted the dev/allichou/UpdateRoslyn branch June 21, 2023 00:03
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