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

counsel.el (counsel-unquote-regex-parens): Bugfixes #1863

Merged
merged 1 commit into from Dec 23, 2018

Conversation

raxod502
Copy link
Contributor

@raxod502 raxod502 commented Dec 20, 2018

Fixes a bug in Counsel that caused radian-software/prescient.el#19.

@mookid
Copy link
Contributor

@mookid mookid commented Dec 20, 2018

Hello Radon,
thanks for the patch. I am trying to understand the issue.

Can you explain the original issue in terms of ivy?

What input was the cause of failure?

Can it be tested to avoid another such regression?

@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented Dec 20, 2018

Well, the actual problem that this resulted in is discussed in the linked issue. As for the cause:

The function counsel-unquote-regex-parens is intended to convert Elisp regexes into PCRE format. This is necessary because Ivy regex functions return Elisp regexes, but external commands such as counsel-rg need PCRE regexes. (Actually, hand-rolling this translation seems like a terrible idea to me, given that pcre2el exists. But that's a separate issue.)

However, it failed to properly unescape \| sequences, so any regex function that returned such regexes wouldn't work with commands like counsel-rg. Also, if the regex function returned a list, then counsel-unquote-regex-parens would fail to do any processing at all, so any regex function that returned a list also wouldn't work with commands like counsel-rg.

As for testing, I don't know what the situation looks like or how best to proceed. TBH, the code for Ivy seems to me like it needs some massive refactoring and rearchitecting. Most of the bugs (and I've fixed a half dozen or more) seem to come from some parts of the codebase working around other parts of the codebase, and not doing so in a sufficiently idiosyncratic way. So, it seems to me like testing is not the optimal way to prevent Ivy regressions, at least right now.

@mookid
Copy link
Contributor

@mookid mookid commented Dec 20, 2018

As for testing, I don't know what the situation looks like or how best to proceed.

You are proposing a change that modifies a pure function that takes either an alist or a string and returns a string, so I guess that

  • writing a test for the change is not that hard
  • it allows to discuss the semantics of your change on a concrete example

Adding such test is probably also a good way to integrate the change faster, making it available for your project.

You noticed that this function is not very precisely documented. Another possible contribution would be to improve the docstring?

As for another ways to fix the quality of the code, I guess that if you have better ideas they are also welcome.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Dec 20, 2018

@raxod502, agreed about refactoring needed. On the other hand, doing that refactoring on top of a state of the code that does not have valid test coverage is troublesome as it would be difficult to verify whether refactored code delivers same/better results as/than the old code does. The least thing one would want to deal with in software engineering is regressions because they unnecessarily set one back while one could spend that time on new features.

@raxod502 raxod502 force-pushed the feat/fix-regex-unquoting branch from 4a00fdf to 4932713 Compare Dec 21, 2018
@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented Dec 21, 2018

I added two tests which would have failed before this change, improved the docstring, and fixed another edge case that was broken before. Let me know if this helps.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 21, 2018

The function counsel-unquote-regex-parens is intended to convert Elisp regexes into PCRE format.

Neither the incumbent name nor its implementation make this clear. In fact, I've refactored this function before and never realised its relevance to PCRE. Perhaps it'd be worth making both this and its internal nature clearer by renaming to counsel--elisp-to-pcre or similar?

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 21, 2018

This is necessary because Ivy regex functions return Elisp regexes, but external commands such as counsel-rg need PCRE regexes.

Ideally this translation shouldn't be necessary or done at all. Rather, it should be possible to determine, for each external command, how to translate user input directly to the tool's accepted format. It just so happens that PCRE is the lingua franca of such tools, so most commands would use the same translation function. No need to limit tools which speak PCRE to using Elisp regexps.

One more thing to add to the refactor/redesign list, I guess.

Actually, hand-rolling this translation seems like a terrible idea to me, given that pcre2el exists. But that's a separate issue.

Hand-rolling is never fun, but FWIW external dependencies aren't a trivial matter, either.

counsel.el Outdated
(lambda (pair)
(let ((subexp (counsel-unquote-regex-parens (car pair))))
(when (string-match-p "|" subexp)
(setq subexp (format "(%s)" subexp)))
Copy link
Collaborator

@basil-conto basil-conto Dec 21, 2018

Choose a reason for hiding this comment

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

Shouldn't this be a shy group (?: ...)?

counsel.el Outdated
(let ((subexp (counsel-unquote-regex-parens (car pair))))
(when (string-match-p "|" subexp)
(setq subexp (format "(%s)" subexp)))
subexp))
Copy link
Collaborator

@basil-conto basil-conto Dec 21, 2018

Choose a reason for hiding this comment

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

Nitpick: why not

(if (string-match-p ...)
    (format ...)
  subexp)

instead?

counsel.el Outdated
(lambda (s)
(or (cdr (assoc s '(("\\(" . "(")
("\\)" . ")")
("(" . "\\(")
(")" . "\\)")
("\\{" . "{")
("\\}" . "}"))))
("\\}" . "}")
("\\|" . "|"))))
Copy link
Collaborator

@basil-conto basil-conto Dec 21, 2018

Choose a reason for hiding this comment

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

Nitpick: adding support for alternatives \| makes both the function's name and the subsequent error message "Unexpected parenthesis" confusing.

Copy link
Collaborator

@basil-conto basil-conto Dec 21, 2018

Choose a reason for hiding this comment

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

(Having said that, the error message would only be printed in what should be an impossible situation.)

The previous code failed to handle the \| operator in regexes, and did
not handle lists of regexes at all.
@raxod502 raxod502 force-pushed the feat/fix-regex-unquoting branch from 4932713 to 58bf1b9 Compare Dec 23, 2018
@raxod502
Copy link
Contributor Author

@raxod502 raxod502 commented Dec 23, 2018

I renamed the function, made the group shy, updated the error message, and made the stylistic change you suggested (although personally I find the original easier to read).

@abo-abo abo-abo merged commit 58bf1b9 into abo-abo:master Dec 23, 2018
1 of 2 checks passed
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 23, 2018

Thanks.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 23, 2018

Regarding refactoring, I agree that some improvements can be done. But I wouldn't say massive.

The core ivy code is covered with integration tests, ivy.el never got into an unusable state over the years. Bugs come in here and there, but it's not common that the same thing comes back over and over. Would be nice to test some counsel.el functionality, but it's quite platform dependent, and depends on external tools.

@raxod502 raxod502 deleted the feat/fix-regex-unquoting branch Dec 23, 2018
yqrashawn added a commit to yqrashawn/.emacs.d that referenced this issue Dec 24, 2018
rgrinberg added a commit to rgrinberg/counsel-projectile that referenced this issue Dec 24, 2018
Re-implemented and renamed in:
abo-abo/swiper#1863

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants