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

move counsel search commands list to variable. Help with extend other… #1661

Closed
wants to merge 3 commits into from
Closed

move counsel search commands list to variable. Help with extend other… #1661

wants to merge 3 commits into from

Conversation

ivasonn
Copy link
Contributor

@ivasonn ivasonn commented Jul 12, 2018

Move counsel search commands list to variable. Help with extend other commands (for example spacemacs/counsel-search)

… commands (for example spacemacs/counsel-search)
ivy.el Outdated
counsel-ag
counsel-rg
counsel-pt)
"counsel search commands")
Copy link
Collaborator

@basil-conto basil-conto Jul 12, 2018

Choose a reason for hiding this comment

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

Please write full sentences, where the first non-keyword word is capitalised and the sentence ends in a full stop. How about the following:

"List of Counsel search commands."

ivy.el Outdated
@@ -253,6 +253,12 @@ It is a list of (CALLER . HEIGHT). CALLER is a caller of
If `(minibuffer-depth)' equals this, `ivy-completing-read' will
act as if `ivy-completing-read-handlers-alist' is empty.")

(defvar counsel--search-commands '(counsel-git-grep
Copy link
Collaborator

@basil-conto basil-conto Jul 12, 2018

Choose a reason for hiding this comment

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

Is there any reason why this variable is defined so far away from its call site? What is this variable's purpose? What other use-cases for this variable are there? Wouldn't it be safer to initialise this variable to nil and have something like

(cl-pushnew #'counsel-git-grep counsel--search-commands)

after every search command is defined?

Copy link
Contributor Author

@ivasonn ivasonn Jul 12, 2018

Choose a reason for hiding this comment

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

  1. That's my fault.
  2. make more customizable, without overriding whole function such as ivy--highlight-default
  3. i am not looking so far, but store commands in variable more correctly. For example after appending new command (like spacemacs did), we don't need edit source code or override func (how i said in previous comment)
  4. i'm agree. it's better.

ivy.el Outdated
@@ -253,6 +253,9 @@ It is a list of (CALLER . HEIGHT). CALLER is a caller of
If `(minibuffer-depth)' equals this, `ivy-completing-read' will
act as if `ivy-completing-read-handlers-alist' is empty.")

(defvar counsel--search-commands nil
"List of Counsel search commands.")
Copy link
Collaborator

@basil-conto basil-conto Jul 12, 2018

Choose a reason for hiding this comment

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

Sorry, I didn't realise I was looking at ivy.el; I thought this was in counsel.el.

Given that this is being used in ivy.el, and given that its intented purpose is as an external API, it has to be given the ivy- (not ivy-- or counsel-) prefix and a name more related to its purpose, e.g. ivy-highlight-search-commands (I'm not very good at naming things, perhaps @abo-abo has a better suggestion).

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 12, 2018

Thanks, I've made some stylistic changes. Please review.

@ivasonn
Copy link
Contributor Author

@ivasonn ivasonn commented Jul 13, 2018

thanks.

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