Skip to content

Fix TextMate missing item #8251

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 10, 2023
Merged

Fix TextMate missing item #8251

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02367b2
Fix TextMate missing item
ryanbrandenburg Feb 9, 2023
e0d3972
Cleanup
ryanbrandenburg Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ...Microsoft.VisualStudio.Razor.IntegrationTests/InProcess/EditorInProcess_Classification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,19 @@ public async Task VerifyGetClassificationsAsync(IEnumerable<ClassificationSpan>
var expectedArray = expectedClassifications.ToArray();
var actualOffset = 0;

for (var i = 0; i < actualArray.Length; i++)
for (var i = 0; i < expectedArray.Length; i++)
Copy link
Member

@phil-allen-msft phil-allen-msft Feb 10, 2023

Choose a reason for hiding this comment

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

I think if actualClassifications << expectedClassifications, this could still throw, but I see how if actual contains "Syntactic" and expected does not, then the new code is the much more likely to succeed state.  You could also have (theoretically) a scenario where there are trailing syntactic classifications  causes the check on L131 to fail.

Would it make sense to filter actualArray into actualSemanticClassifications with the clause on L104 (Linq.Where or similar), check the lengths, and then proceed through the equal length arrays?

Copy link
Contributor Author

@ryanbrandenburg ryanbrandenburg Feb 10, 2023

Choose a reason for hiding this comment

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

I like the Where suggestion, much cleaner. I still prefer that the length is checked at the bottom so that we proceed through the arrays before checking length (ie, finding out what area is messed up is more informative than finding out the difference in expected length).

{
var actualClassification = actualArray[i+actualOffset];
var expectedClassification = expectedArray[i];

if (actualClassification.ClassificationType.BaseTypes.Count() == 1 && actualClassification.ClassificationType.BaseTypes.Any(t => t is ILayeredClassificationType layered && layered.Layer == ClassificationLayer.Syntactic))
if (actualClassification.ClassificationType.BaseTypes.Count() == 0 &&
actualClassification.ClassificationType is ILayeredClassificationType layeredClassificationType &&
layeredClassificationType.Layer == ClassificationLayer.Syntactic)
{
// Don't check purely syntactic classifications, we're gonna try this again.
actualOffset++;
actualClassification = actualArray[i+ actualOffset];
i--;
continue;
}

if (actualClassification.ClassificationType.BaseTypes.Count() > 1)
Expand All @@ -124,7 +128,7 @@ public async Task VerifyGetClassificationsAsync(IEnumerable<ClassificationSpan>
}
}

Assert.Equal(expectedArray.Length, actualArray.Length);
Assert.Equal(expectedArray.Length, actualArray.Length - actualOffset);
}

public async Task<IEnumerable<ClassificationSpan>> GetClassificationsAsync(CancellationToken cancellationToken)
Expand Down