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

FindTextInFiles doesn't support comma-separated globs with {} #169885

Merged
merged 10 commits into from
Feb 7, 2023

Conversation

andreamah
Copy link
Contributor

@andreamah andreamah commented Dec 22, 2022

Fixes #169422

note: doesn't support .. notation within braces.

@andreamah andreamah self-assigned this Dec 22, 2022
@andreamah andreamah dismissed a stale review via 1b128ae February 3, 2023 01:16
@andreamah andreamah marked this pull request as ready for review February 3, 2023 22:28
@vscodenpa vscodenpa added this to the February 2023 milestone Feb 3, 2023
function spreadGlobComponents(globArg: string): string[] {
const components = splitGlobAware(globArg, '/');
return components.map((_, i) => components.slice(0, i + 1).join('/'));
function spreadGlobComponents(globComponent: string): string[] {
Copy link
Member

Choose a reason for hiding this comment

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

As the glob processing gets more complicated, it would probably be helpful to break out the code in getRgArgs with the !* etc that handles glob patterns and write a unit test for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now, just added unit tests for that part of getRgArgs

escaped = false;
} else {
if (inBraces) {
// ripgrep treats this as attempting to do an alternating group, which is invalid. Return with pattern including changes from escaped braces.
Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, this shouldn't be a ripgrep-specific function. glob.ts is our implementation of glob patterns and we should do what's right for our implementation. If it's really "expandBracePatternsForRipgrep" then it can live somewhere else

escaped = false;
} else if (inBraces) {
// we found an end bracket to a valid opening bracket. Return the appropriate strings.
return { fixedStart, strInBraces, fixedEnd: pattern.substring(i + 1) };
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean you can only have one {} pattern per glob? Is that the rule in glob.ts in general or in ripgrep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function finds the first valid { and } pair, but the caller calls this function again on the trailing part of the string to parse the rest of it

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

I think this looks good!

@andreamah andreamah merged commit 16e26b4 into main Feb 7, 2023
@andreamah andreamah deleted the andreamah/issue169422 branch February 7, 2023 20:06
c-claeys pushed a commit to c-claeys/vscode that referenced this pull request Feb 16, 2023
…osoft#169885)

* `FindTextInFiles` doesn't support comma-separated globs with {}
Fixes microsoft#169422
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 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.

FindTextInFiles doesn't support comma-separated globs with {}
4 participants