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

ivy.el (ivy--occur-insert-lines): fix incorrect regexp. #1846

Closed
wants to merge 1 commit into from

Conversation

mookid
Copy link
Contributor

@mookid mookid commented Dec 8, 2018

Currently the . seems too permissive and should be escaped.

the correct one was given by @basil-conto in #1817 (comment) of course (:

@mookid mookid force-pushed the fix_ivy_occur_insert_lines_re branch from 738dbb1 to 32a5e69 Compare Dec 8, 2018
ivy-test.el Outdated
(should
(equal
'((ok) (ok))
(delete
Copy link
Collaborator

@basil-conto basil-conto Dec 8, 2018

Choose a reason for hiding this comment

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

delete can be delq here.

ivy-test.el Outdated
(cons "test3" 'fail)
(cons "t/est4" 'fail)
(cons "t\\est5" 'fail)
(cons "tes./t6" 'fail)))))))
Copy link
Collaborator

@basil-conto basil-conto Dec 8, 2018

Choose a reason for hiding this comment

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

Nitpick: Strings and symbols are self-quoting, and we're not modifying the alist, so a literal '(("..." . ok) ...) would suffice here.

Having said that, I think this test can be written in a more straight-forward way:

(mapc (lambda (p)
        (should (eq (and (ivy--starts-with-dotslash (car p)) t)
                    (cdr p))))
      '(("./test1" . t)
        (".\\test2" . t)
        ("test3")
        ("t/est4")
        ("t\\est5")
        ("tes./t6")))

or similar. Having said that, it is more idiomatic to keep logic to a minimum in ERT tests:

(should (ivy--starts-with-dotslash "./test1"))
(should (ivy--starts-with-dotslash ".\\test2"))
(should (not (ivy--starts-with-dotslash "test3")))
(should (not (ivy--starts-with-dotslash "t/est4")))
(should (not (ivy--starts-with-dotslash "t\\est5")))
(should (not (ivy--starts-with-dotslash "tes./t6")))

Although I'm not usually a fan of repeating myself, tests are an exception where the latter style is instantly more obvious and possibly produces cleaner backtraces (though I'm not sure about this last point).

Copy link
Contributor Author

@mookid mookid Dec 8, 2018

Choose a reason for hiding this comment

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

I did not really pay much attention to the way these were written to be honest.
I like the last option more.

ivy--starts-with-dotslash: new function.
@mookid mookid force-pushed the fix_ivy_occur_insert_lines_re branch from 32a5e69 to dd019b0 Compare Dec 8, 2018
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 12, 2018

Thanks.

@mookid mookid deleted the fix_ivy_occur_insert_lines_re branch Dec 12, 2018
basil-conto added a commit that referenced this issue Apr 25, 2020
(ivy--dirname-p, ivy--magic-file-slash): Simplify now that Ivy
requires Emacs 24.5 or later by using string-suffix-p instead of
slower regexp matching.
(ivy--occur-insert-lines): Escape leading dot in regexp by using
existing helper ivy--starts-with-dotslash.

Re: #1817, #1845, #1846
astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 2021
(ivy--dirname-p, ivy--magic-file-slash): Simplify now that Ivy
requires Emacs 24.5 or later by using string-suffix-p instead of
slower regexp matching.
(ivy--occur-insert-lines): Escape leading dot in regexp by using
existing helper ivy--starts-with-dotslash.

Re: abo-abo#1817, abo-abo#1845, abo-abo#1846
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

3 participants