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--grep-mode-occur: remove column indicators for wgrep. #1835

Closed
wants to merge 1 commit into from

Conversation

mookid
Copy link
Contributor

@mookid mookid commented Dec 2, 2018

Fix #1800.

@zilongshanren: can you please test if this solves your issue?

counsel.el Outdated
(save-match-data
(if (string-match
;; Remove column indicators if present
"\\(?1:[^\n:]+?[^\n/:]\\):[\t ]*\\(?3:[1-9][0-9]*\\)[\t ]*:\\(?3:[1-9][0-9]*:\\)" cand)
Copy link
Collaborator

@basil-conto basil-conto Dec 2, 2018

Choose a reason for hiding this comment

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

Why are there three explicitly numbered groups, when only the last one is going to be used? (When there are multiple capture groups with the same number, only the right-most one is used.)

Copy link
Contributor Author

@mookid mookid Dec 3, 2018

Choose a reason for hiding this comment

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

I borrowed that from grep.el. let's rework the code after confirmation that the fix works.

counsel.el Outdated
(if (string-match
;; Remove column indicators if present
"\\(?1:[^\n:]+?[^\n/:]\\):[\t ]*\\(?3:[1-9][0-9]*\\)[\t ]*:\\(?3:[1-9][0-9]*:\\)" cand)
(replace-match "" nil nil cand 3)
Copy link
Collaborator

@basil-conto basil-conto Dec 2, 2018

Choose a reason for hiding this comment

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

The replace-match arguments FIXEDCASE and LITERAL should be t.

counsel.el Outdated
;; Remove column indicators if present
"\\(?1:[^\n:]+?[^\n/:]\\):[\t ]*\\(?3:[1-9][0-9]*\\)[\t ]*:\\(?3:[1-9][0-9]*:\\)" cand)
(replace-match "" nil nil cand 3)
cand)
Copy link
Collaborator

@basil-conto basil-conto Dec 2, 2018

Choose a reason for hiding this comment

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

I wonder if this can be simplified using replace-regexp-in-string?

counsel.el Outdated
"\\(?1:[^\n:]+?[^\n/:]\\):[\t ]*\\(?3:[1-9][0-9]*\\)[\t ]*:\\(?3:[1-9][0-9]*:\\)" cand)
(replace-match "" nil nil cand 3)
cand)
(concat "./" prepend cand))) ivy--all-candidates))))
Copy link
Collaborator

@basil-conto basil-conto Dec 2, 2018

Choose a reason for hiding this comment

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

Nitpick: Please place ivy--all-candidates on a separate line, where it won't be hidden.

@zilongshanren
Copy link

@zilongshanren zilongshanren commented Dec 3, 2018

@mookid thanks, I have tested the patch, it seems not fixing my problem.

I think this patch should be applied to the function counsel-grep-like-occur .

@mookid
Copy link
Contributor Author

@mookid mookid commented Dec 3, 2018

@zilongshanren thanks for the feedback.

I think this patch should be applied to the function counsel-grep-like-occur .

Probably to all 3 callers of ivy--occur-insert-lines in counsel.el indeed.

@zilongshanren
Copy link

@zilongshanren zilongshanren commented Dec 3, 2018

@mookid
I manually copy the patch code to counsel-grep-like-occur fun, but it is not working :(

@mookid
Copy link
Contributor Author

@mookid mookid commented Dec 3, 2018

I will take a look at it later today.

@mookid
Copy link
Contributor Author

@mookid mookid commented Dec 4, 2018

@zilongshanren could you please test again? thanks.

@zilongshanren
Copy link

@zilongshanren zilongshanren commented Dec 5, 2018

@mookid Now it works, thanks. 👍

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 13, 2018

Thanks all.

abo-abo added a commit that referenced this issue Dec 13, 2018
@mookid mookid deleted the counsel-ag-wgrep branch Dec 13, 2018
astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 2021
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

4 participants