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

Add ripgrep pcre2 search support #19615

Merged
merged 1 commit into from Jul 1, 2019

Conversation

@dwelle
Copy link
Contributor

commented Jun 30, 2019

Issue or RFC Endorsed by Atom's Maintainers

Suggestion discussed in this PR.

Fixes atom/find-and-replace#571

Description of the Change

Added option to enable pcre2 ripgrep regex engine for search.

Alternate Designs

None applicable.

Possible Drawbacks

Enabling PCRE2 potentially incurs performance penalty, as explained on ripgrep's wiki, thus this PR depends on a new atom/find-and-replace PCRE2 option, being added in this PR.

Verification Process

I haven't verified anything because I haven't managed to build Atom ATM. Considering the nature of the change, I'm relying on the CI. If this is not acceptable, feel free to close or keep the PR open until I manage to build Atom locally, but I give no promises as to when I'll get the time to do so.

Release Notes

  • Added support to enable pcre2 for ripgrep search engine in find-and-replace package to enable advanced regex features such as lookbehind.
@dwelle dwelle referenced this pull request Jun 30, 2019
spec/workspace-spec.js Outdated Show resolved Hide resolved
@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Thanks for the contribution!

I haven't verified anything because I haven't managed to build Atom ATM. Considering the nature of the change, I'm relying on the CI. If this is not acceptable, feel free to close or keep the PR open until I manage to build Atom locally, but I give no promises as to when I'll get the time to do so.

That's fine 😃 As long as CI passes it should be ok 😄

Currently there's a linter error, these errors are fixed it by running script/lint --fix (or by integrating prettier on Atom), but if you haven't been able to bootstrap the environment correctly, that script may not work.

I've added a suggestion that I think should fix the error based on the CI error message.

Codewise looks good to me, so once the CI build passes we can merge this one 😄

@dwelle

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Seems I forgot to pass PCRE2: true in specs --- fixed. Oh the perils of coding blind :).

@dwelle

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

lint again.. should I squash those commits?

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

lint again.. should I squash those commits?

👍

@dwelle dwelle force-pushed the dwelle:ripgrep-pcre2 branch from bad0a52 to 5905876 Jul 1, 2019

Add ripgrep pcre2 support
👕 fix lint

Co-Authored-By: Rafael Oleza <rafeca@gmail.com>

fix passing PCRE2 flag in specs

👕 fix lint

@dwelle dwelle force-pushed the dwelle:ripgrep-pcre2 branch from 5905876 to ef7b910 Jul 1, 2019

@dwelle

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Removed expectations for leadingContextLines/trailingContextLines since I guess it's not necessary to assert context for this spec.

@rafeca

rafeca approved these changes Jul 1, 2019

Copy link
Contributor

left a comment

🤗

@rafeca rafeca merged commit 9ae30a3 into atom:master Jul 1, 2019

1 check passed

Atom Pull Requests #20190701.11 succeeded
Details
@BurntSushi

This comment has been minimized.

Copy link

commented Jul 6, 2019

Y'all might also be also be interested in ripgrep's --auto-hybrid-regex flag, which was added in ripgrep 11:

       --auto-hybrid-regex
           When this flag is used, ripgrep will dynamically choose
           between supported regex engines depending on the features
           used in a pattern. When ripgrep chooses a regex engine, it
           applies that choice for every regex provided to ripgrep
           (e.g., via multiple -e/--regexp or -f/--file flags).

           As an example of how this flag might behave, ripgrep will
           attempt to use its default finite automata based regex
           engine whenever the pattern can be successfully compiled
           with that regex engine. If PCRE2 is enabled and if the
           pattern given could not be compiled with the default regex
           engine, then PCRE2 will be automatically used for searching.
           If PCRE2 isn’t available, then this flag has no effect
           because there is only one regex engine to choose from.

           In the future, ripgrep may adjust its heuristics for how it
           decides which regex engine to use. In general, the
           heuristics will be limited to a static analysis of the
           patterns, and not to any specific runtime behavior observed
           while searching files.

           The primary downside of using this flag is that it may not
           always be obvious which regex engine ripgrep uses, and thus,
           the match semantics or performance profile of ripgrep may
           subtly and unexpectedly change. However, in many cases, all
           regex engines will agree on what constitutes a match and it
           can be nice to transparently support more advanced regex
           features like look-around and backreferences without
           explicitly needing to enable them.

           This flag can be disabled with --no-auto-hybrid-regex.
@dwelle

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@BurntSushi thanks for heads-up. I see vscode recently implemented it as well, with a plan to deprecate --pcre2 flag altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.