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

[feat] New customizations: helm-swoop-flash-region-function #146

Merged
merged 1 commit into from
Aug 13, 2019

Conversation

cireu
Copy link
Collaborator

@cireu cireu commented Aug 13, 2019

No description provided.

@conao3
Copy link
Collaborator

conao3 commented Aug 13, 2019

LGTM.

Copy link
Collaborator

@conao3 conao3 left a comment

Choose a reason for hiding this comment

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

Thanks, PR! Please check those comments.

helm-swoop.el Outdated
@@ -524,7 +535,7 @@ If $linum is number, lines are separated by $linum"
(when (or (and (and (featurep 'migemo) helm-migemo-mode)
(migemo-forward $regex nil t))
(re-search-forward $regex nil t))
(helm-swoop-flash-word (match-beginning 0) (match-end 0))
(helm-swoop-do-flash-region (match-beginning 0) (match-end 0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

(funcall helm-swoop-flash-region-function (match-beginning 0) (match-end 0))

instead of define helm-swoop-do-flash-region?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not a small util function? At least it looks meaningful than direct calling funcall.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Increasing small utility functions requires a lot of consideration.

For example, all functions declared with defsubst seem to be prefixed with helm-swoop--, but how about renaming them?
And In many cases, I think do is unnecessary.

helm-swoop.el Outdated Show resolved Hide resolved
helm-swoop.el Outdated Show resolved Hide resolved
@conao3
Copy link
Collaborator

conao3 commented Aug 13, 2019

[Offtopic]
And #145 includes #138, don't you?
I suggest revert all #145, and merge #138, then you can more work on it.

Currently, the contribution information of the person who wrote #145 from the history of git is missing and it is not a favorable situation.

@cireu
Copy link
Collaborator Author

cireu commented Aug 13, 2019

[Offtopic]
And #145 includes #138, don't you?
I suggest revert all #145, and merge #138, then you can more work on it.

Currently, the contribution information of the person who wrote #145 from the history of git is missing and it is not a favorable situation.

See #148

@cireu
Copy link
Collaborator Author

cireu commented Aug 13, 2019

@conao3 OK now.

@cireu cireu merged commit 5e2e3ce into master Aug 13, 2019
@cireu cireu deleted the orp/custom-highlight-fn branch August 13, 2019 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants