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

Improve swiper handling of invalid regexes #1545

Closed

Conversation

@muirmanders
Copy link
Contributor

@muirmanders muirmanders commented Apr 28, 2018

Now when splitting apart the input by space, we always regex escape
each part if it is an invalid regex.

For regex-ignore-order, this fixes a bug where in swiper you
couldn't hit RET to go to the line of the selected match if your input
was an invalid regex. ignore-order had logic to escape invalid
regexes, but swiper calls ivy--regex on the input, which previously
was not performing the same escaping.

For regex-plus, you now also get a literal search if you enter an
invalid regex (previously it silently failed to search). It seems like
this is an improvement in behavior, but perhaps it will annoy
regex-plus users.

Fixes #1516

Now when splitting apart the input by space, we always regex escape
each part if it is an invalid regex.

For regex-ignore-order, this fixes a bug where in swiper you
couldn't hit RET to go to the line of the selected match if your input
was an invalid regex. ignore-order had logic to escape invalid
regexes, but swiper calls ivy--regex on the input, which previously
was not performing the same escaping.

For regex-plus, you now also get a literal search if you enter an
invalid regex (previously it silently failed to search). It seems like
this is an improvement in behavior, but perhaps it will annoy
regex-plus users.
@muirmanders
Copy link
Contributor Author

@muirmanders muirmanders commented Apr 28, 2018

I have (finally) completed my copyright assignment to Emacs. I can provide the signature file if necessary.

Copy link
Collaborator

@basil-conto basil-conto left a comment

Some very minor nitpicks.

ivy.el Outdated
@@ -2261,7 +2261,8 @@ This concept is used to generalize regular expressions for
(defun ivy--split (str)
"Split STR into a list by single spaces.
The remaining spaces stick to their left.
This allows to \"quote\" N spaces by inputting N+1 spaces."
This allows to \"quote\" N spaces by inputting N+1 spaces.
Each part is regex escaped if it is not a valid regex."

This comment has been minimized.

@basil-conto

basil-conto Apr 29, 2018
Collaborator

I think this whole docstring can be written a bit more clearly, but I don't feel very confident with wording. Here's an alternative take:

  "Split STR into list of substrings bounded by spaces.
Single spaces act as splitting points.  Consecutive spaces
\"quote\" their preceding spaces, i.e., guard them from being
split.  This allows the literal interpretation of N spaces by
inputting N+1 spaces.  Any substring not constituting a valid
regexp is passed to `regexp-quote'."

WDYT?

ivy.el Outdated
@@ -2295,7 +2296,7 @@ This allows to \"quote\" N spaces by inputting N+1 spaces."
(setq s (substring str start1))
(unless (= (length s) 0)
(push s res)))
(nreverse res)))
(mapcar 'ivy--regex-or-literal (nreverse res))))

This comment has been minimized.

@basil-conto

basil-conto Apr 29, 2018
Collaborator

Please quote function symbols with #' like you do in other parts of this PR.

ivy.el Outdated
@@ -2364,7 +2365,8 @@ text after delimiter if it is empty. Modifies match data."

(defun ivy--split-spaces (str)
"Split STR on spaces, unless they're preceded by \\.
No unescaped spaces are present in the output."
No unescaped spaces are present in the output.
Each part is regex escaped if it is not a valid regex."

This comment has been minimized.

@basil-conto

basil-conto Apr 29, 2018
Collaborator

Again, a minor wording suggestion:

  "Split STR on spaces, unless they're preceded by \\.
No unescaped spaces are left in the output.  Any substring not
constituting a valid regexp is passed to `regexp-quote'."
@abo-abo abo-abo closed this in 5c0b848 May 6, 2018
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 6, 2018

Thanks for the contribution.

I see your name on copyright.list with an email different from the one you used on the commit. This is fine with me, but it might be something worth updating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants