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

[Core] Adding swiper-helm for re-search in helm #4862

Closed

Conversation

dcluna
Copy link
Contributor

@dcluna dcluna commented Jan 29, 2016

Adding swiper-helm to 'Search' menu for regexp-based search with helm.

@dcluna dcluna changed the title Adding swiper-helm for re-search in helm [Core] Adding swiper-helm for re-search in helm Jan 29, 2016
@robbyoconnor
Copy link
Contributor

👍

(defun spacemacs/swiper-helm-region-or-symbol ()
(interactive)
(swiper-helm
(if (region-active-p)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy-pasted this code because I was not sure where to extract a private function (like spacemacs--region-or-symbol-at-point) to. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit simpler:

(if (region-active-p)
    (buffer-substring-no-properties (region-beginning)
                                    (region-end))
  (or (thing-at-point 'symbol t) ""))

@syl20bnr
Copy link
Owner

syl20bnr commented Feb 1, 2016

What's the difference with SPC s s ? :-)

@dcluna
Copy link
Contributor Author

dcluna commented Feb 3, 2016

@syl20bnr Not much, I guess...closing this

@dcluna dcluna closed this Feb 3, 2016
@robbyoconnor
Copy link
Contributor

Why close this? Don't give up so easily...defend it!

@dcluna
Copy link
Contributor Author

dcluna commented Feb 5, 2016

@robbyoconnor Well, this one works with numbers and is faster when specifying regexes...not sure this is enough grounds to reopen this though. Maybe if I specify an option, like in ruby-mode?

@dcluna dcluna reopened this Feb 5, 2016
@dcluna dcluna force-pushed the add_swiper_helm_core_spacemacs branch 2 times, most recently from 23e744c to 805d200 Compare February 5, 2016 21:02
@dcluna
Copy link
Contributor Author

dcluna commented Feb 5, 2016

Reopened, now it's a user-configurable option. Can you review again? cc @robbyoconnor @syl20bnr


(spacemacs/set-leader-keys
"ss" 'swiper-helm
"sS" 'spacemacs/swiper-helm-region-or-symbol))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now these two substitute helm-swoop by user choice.

@robbyoconnor
Copy link
Contributor

I have no objections, but @syl20bnr is the boss

@@ -171,6 +171,9 @@ start.")
(defvar dotspacemacs-helm-position 'bottom
"Position in which to show the `helm' mini-buffer.")

(defvar dotspacemacs-helm-use-swiper nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be a variable local to the layer, defined in a file config.el.

@syl20bnr
Copy link
Owner

syl20bnr commented Feb 6, 2016

No need for a variable, you can add (helm-swoop :excluded t) to the list of packages of the layer and it will automatically remove helm-swoop effectively replacing helm-swoop by swiper-helm.

@robbyoconnor
Copy link
Contributor

☝️ Doooo it DOOOOOOOOOOOOOOOOOO IT

@dcluna dcluna force-pushed the add_swiper_helm_core_spacemacs branch 2 times, most recently from 1401321 to 4f06c1e Compare February 8, 2016 16:11
@dcluna
Copy link
Contributor Author

dcluna commented Feb 8, 2016

Done, hope it's fine now

@dcluna
Copy link
Contributor Author

dcluna commented Feb 24, 2016

@robbyoconnor @syl20bnr Is this what you mentioned?

@dcluna dcluna force-pushed the add_swiper_helm_core_spacemacs branch from 4f06c1e to 9210807 Compare March 3, 2016 20:50
@deb0ch
Copy link
Contributor

deb0ch commented Oct 16, 2016

Autumnal cleanup: Needs conflicts resolution. What is the status on this PR?

@dcluna
Copy link
Contributor Author

dcluna commented Oct 5, 2017

I don't think this PR makes sense anymore, I'll close it and come back if I ever see a need for this package again.

@dcluna dcluna closed this Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants