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

Escape special glob chars when using "Find in Folder" #166318

Merged
merged 9 commits into from
Jan 31, 2023

Conversation

davidnx
Copy link
Contributor

@davidnx davidnx commented Nov 14, 2022

Fixes #82415

@davidnx
Copy link
Contributor Author

davidnx commented Nov 14, 2022

For inspiration, see also: Python's glob.escape function.
Their docs indicate only ?, *, and [ need to be escaped, but tests with VS Code globbing indicated that ] too needed escaping. In this PR, I am handling all four, and converting them to character range sequences.

@andreamah
Copy link
Contributor

Thanks for the contribution!

Trying this out, it might be a bit confusing to see extra square brackets in the "files to include" field when you want to search within them. Rather, would it be possible to change the way that the path gets interpreted the search actually happens? I believe that it happens around here:

private handleIncludeExclude(pattern: string | string[] | undefined, expandPatterns: boolean | undefined): ISearchPathsInfo {

That way, we don't see any difference in the UI.

Another thing: I tried it out and I'm still getting results from others folders? Unsure why.
image

@davidnx
Copy link
Contributor Author

davidnx commented Jan 26, 2023

@andreamah The screenshot you sent is indeed perplexing, though if it is a legit bug I don't think it's related to my changes -- perhaps the existing globbing functionality is broken on some platforms? Seems unlikely, but I can't think of another reason why it wouldn't be working on your side. Can you share which platform or OS you tried it on? Below are my results on Windows, and everything works as expected:

  • Folder structure I am using for the test (I tried to mimic what you had based on the limited information from your comment):
    image

  • And here are results showing search works correctly with the escaped glob pattern that my change produced:
    image

As you can see, only the results in subfolder[new] appear, as desired. I don't know why it behaved differently in your tests, and I would suggest it may be a separate bug on globbing, no on how this PR is escaping glob patterns. This PR looks correct as far as I can tell.


Re. your comment:

it might be a bit confusing to see extra square brackets in the "files to include" field when you want to search within them. Rather, would it be possible to change the way that the path gets interpreted the search actually happens?

I think what you're proposing is equivalent to saying "let's get rid of globbing as the mechanism to specify search paths in VS Code", which would be a very significant change, likely undesirable, and certainly out of scope for this PR. Globbing is a fine and commonplace choice for search tools, and it requires escaping special characters. I don't see any way around that... This PR simply implemsnts escaping properly, which was missing before.

Let me know how I can help further, I think this PR will improve the experience of VSCode users.

Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

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

Thinking this over again, it seems to make a lot of sense. I also didn't run into the bug that I ran into last time, so I'll just bring it up again if I see it again.

Thanks for the contribution!!

@andreamah andreamah merged commit 9f6c013 into microsoft:main Jan 31, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"files to include" doesn't work with folder names containing glob characters
5 participants