-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Fix #59364 : Respect global .gitignore when searching #59717
Conversation
This is a good start, but it needs to be hooked up to a setting so users can use it :) Follow the example of the Then the flag on the query also needs to be checked in the extension version of search - https://github.com/Microsoft/vscode/blob/master/extensions/search-rg/src/ripgrepTextSearch.ts#L350 |
@roblourens , Thanks for the review :) |
@roblourens , Please check the changes now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do need to use the setting somewhere, currently it is not hooked up to anything. For example the useIgnoreFiles setting is used here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/search/common/queryBuilder.ts#L63
@roblourens , I have followed the exact way that search.useIgnoreFiles was implemented.(Taking into account both the review feedbacks that you gave). |
It looks like the build has an issue |
I checked that before pushing and all the errors are related to test files and few of them are due to the "disregardGlobalIgnoreFiles" flag that was added and others were related to something else, I did not know if I had to fix that build issue. |
Hey @roblourens , sorry for that build failure error, it was my bad. |
I added one comment to the code but everything else looks good. But the QueryBuilder tests are still failing because now this extra property is added to the query. So could you just update them so they pass? Then I see CompletionModel tests also failing, I'm not sure about those. You could merge the latest from master into your PR branch to check whether it's something else from master or your code. If it's not from your change we can ignore that one... |
@roblourens , Fixed all the changes and QueryBuilder tests are now working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks!
@roblourens , Here is the PR that fixes #59364 .
Please review the same and let me know if any changes are needed.
Thanks :)