Closed rafeca closed 5 years ago
Going to merge this one, if there's any feedback afterwards I'll handle it on a separate PR
As @maxbrunsfeld has mentioned on Slack, the ripgrep
scanner also fixes https://github.com/atom/find-and-replace/issues/806, which is a long-standing issue regarding not ignoring VCS-ignored files when there's a filter path in the project search.
Scanning the commit, it'd be great to allow to enable ripgrep's PCRE2 so that lookbehind assertions work, too (https://github.com/atom/find-and-replace/issues/571). AFAIK it has a small perf regression, so if that's a concern, best adding it as opt-in only (ala vscode).
@dwelle thanks for the suggestion!
Would you be willing to contribute adding this as a config option?
Sure, provided it's not time-sensitive because I reckon it'll take a while --- I'll have to set up a dev enviro and figure out how to run it (plus I have little time ATM, but who does). Alternatively, I can write the code + tests blindly, cross my fingers and let the CI and reviewers do the rest :).
Btw, should a new event metadata field be added, too?
Thanks @dwelle! I don't think time sensitive, so it's fine 😄
Setting up the dev environment is quite easy, you can follow the instructions here and here, but if you prefer you can let the CI do the job 😭
Just take into account that the search logic is in Atom core. You can add a new option to the [workspace.scan()
method] (https://github.com/atom/atom/blob/master/src/workspace.js#L2048) to use PCRE2, and then pass it from find-and-replace
based on the config option that you create (as I did for ripgrep
).
You don't need to care about the DefaultDirectorySearcher
implementation (just mention on the configuration option description that it only works on ripgrep
).
Also, make sure to create a new test on the workspace-spec.js
file that asserts that the PCRE2 option works well (you can use any existing test as inspiration).
Regarding the metadata field, I don't think it's needed for now. Only if we see that the performance of the searches regress after this change we'll add that.
Thanks again for your help!
@rafeca finally managed to free up some time to work on this.
Being on Win7 I've been trying to bootstrap the atom repo for quite some time but it keeps failing on the well known msbuild.exe failed with exit code: 1
error.
I believe I have the correct version of Visual C++ Build Tools
, but as for the Windows 8 SDK
, not sure. The installation shows as successful, but then doesn't show as installed when trying to install it again (to verify).
I'll try to re-install everything, but if I don't break through soon, I'll have to go with my original idea and write the code blind.
Yaaaaas! Thank you so much. Alleviates one of the prime badgering points that my co-workers love to make fun of me for when I say that I still use atom after years of being a dev.
Summary
This PR adds a config option to enable
ripgrep
powered find and replace on this package. The integration withripgrep
has been done in Atom core on https://github.com/atom/atom/pull/19348 and this PR just opts in the find and replace package to use it.Benefits
Using
ripgrep
speeds up drastically the time to find files on any kind of repository (we're taking about up to 22X faster times):(the last measure has been done to check the less favourable case for
ripgrep
, which is a search that returns a lot of results (26k) which need to be passed from theripgrep
process to Atom. This is still 8X times faster than the current search logic. We could limit the number of results, but this can be done as a separate PR).Possible Drawbacks
The search behaviour with
ripgrep
is slightly different than the one currently implemented, and there may be still some edge cases to polish. There's more information about the changes in https://github.com/atom/atom/pull/19348.To mitigate that, I've done quite extensive tests but I'll keep checking for potential edge cases. Also, I'm planning to leave this config flag at least for 1 version of Atom so we can catch as many issues as possible before shipping this to users by default .
Applicable Issues
https://github.com/atom/find-and-replace/issues/1075