Skip to content

Conversation

@majocha
Copy link
Contributor

@majocha majocha commented Feb 13, 2023

We have a very old copy of PatternMatcher from Roslyn repo in use by FSharpNavigateToSearchService.
Apparently there is a version of it provided by VS SDK now: IPatternMatcher Interface

I replaced the old PatternMatcher and manually verified:

  • Ctrl-T search still works as before
  • performance is not worse

One noticeable change is that now camel case search does not require typing in allcaps, which is in line with how C# search works.

@majocha majocha requested a review from a team as a code owner February 13, 2023 22:53
@dnfadmin
Copy link

dnfadmin commented Feb 13, 2023

CLA assistant check
All CLA requirements met.

@T-Gro
Copy link
Member

T-Gro commented Feb 14, 2023

"One noticeable change is that now camel case search does not require typing in allcaps:"
Could this change of behavior be covered in a test please?

@majocha
Copy link
Contributor Author

majocha commented Feb 14, 2023

"One noticeable change is that now camel case search does not require typing in allcaps:" Could this change of behavior be covered in a test please?

I could try, but I don't think we have any tests for NavigateToSearchService?

@psfinaki
Copy link
Contributor

@majocha it's never too late to start adding them :) I will also see if we can have any integration testing here.

Thanks for this! I think I replaced another copy of it back in a day but missed this somehow.

@majocha
Copy link
Contributor Author

majocha commented Feb 14, 2023

@psfinaki, I'm afraid I've hit a roadblock regarding testing this.
There seems to be a mismatch between roslyn and our repo.

Rolsyn still provides internals only to VisualFSharp.UnitTests https://github.com/dotnet/roslyn/blob/0d6ace528d0a142ab51be350e05e651fd053b629/src/Tools/ExternalAccess/FSharp/Microsoft.CodeAnalysis.ExternalAccess.FSharp.csproj#L24)

In effect while FSharp.Editor.Tests does import Microsoft.CodeAnalysis.ExternalAccess.FSharp, it can't see the types needed here.

@psfinaki
Copy link
Contributor

@majocha oops alright yeah thanks for letting us know. I created a PR to fix this but even if they approve it will take the changes a few days to propagate.

We can add tests as a followup.

@psfinaki
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor

So integration testing should be possible here. Here is how it might look like: #14764
But that's blocked by dotnet/roslyn#66926 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants