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

Interactively specify switches #1559

Closed
wants to merge 1 commit into from

Conversation

@DEADB17
Copy link
Contributor

commented May 6, 2018

Closes #1408

Purpose

Enable users to interactively specify extra switches along with the search pattern when invoking counsel-ag.

For example, M-x counsel-ag:

[~/.emacs.d] ag: --elisp --ignore site-lisp -- let

UI

Assume counsel-ag-command is set to ag --nocolor --nogroup.

  • It behaves the same as before when the query starts with anything but a dash:
    M-x counsel-ag abc
    Produces ag --nocolor --nogroup abc

  • Regex are escaped:
    M-x counsel-ag abc[^d].
    Produces ag --nocolor --nogroup abc\[\^d\].

  • Starting with - or -- passes the text as switches until a --:
    M-x counsel-ag --elisp -- abc
    Produces ag --nocolor --nogroup --elisp abc

  • Initial dashes need to be escaped if they are not switches:
    M-x counsel-ag \-abc
    Produces ag --nocolor --nogroup \\-abc

  • Switches are not escaped. Queries are:
    M-x counsel-ag --elisp -- abc[^d].
    Produces ag --nocolor --nogroup --elisp abc\[\^d\].

  • Using a prefix argument behaves the same as before:
    C-u M-x counsel-ag ag --nocolor --nogroup --elisp
    abc
    Produces ag --nocolor --nogroup --elisp abc

Commentary

Except for the two bugs below it seems to work OK, but more testing is welcome.
I'm looking for early feedback on the code and functionality and any bugs that I might have missed.

I don't have rip-grep so I don't know how it interacts with counsel-rg. It looks like it works fine with counsel-rg.

How to test

  1. Clone the repo.
  2. Check out dash-dash-opts.
  3. Open ivy.el and M-x emacs-lisp-byte-compile-and-load.
  4. Open counsel.el and M-x emacs-lisp-byte-compile-and-load.
  5. Call ag: M-x counsel-ag.

Known bugs

  • Fixed: ivy-occur is broken.
  • Fixed: The minimum character count in the ag-prompt is negative when switches are used.

Contribution checklist

  • make compile
  • make test
  • make checkdoc

Maybe

  • Add tests for the new functionality
  • Add user documentation for the new functionality
counsel.el Outdated
@@ -2245,18 +2245,50 @@ regex string."
(ivy-set-occur 'counsel-ag 'counsel-ag-occur)
(ivy-set-display-transformer 'counsel-ag 'counsel-git-grep-transformer)

(defconst user-input-switch "-- ")

This comment has been minimized.

Copy link
@abo-abo

abo-abo May 8, 2018

Owner

Please prefix all variables and functions with counsel-.

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 9, 2018

Author Contributor

Done

counsel.el Outdated
"Grep in the current directory for STRING."
(let ((parsed (parse-user-input string)))
(let ((query (car parsed))
(extra-ag-args (cdr parsed)))

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 8, 2018

Collaborator

You can use a single let* instead of nested let.

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 8, 2018

Author Contributor

I though that since query and extra-ag-args don't depend on each other, only on parsed it'd make the relationship clearer. I can switch it to let* if reducing visual nesting makes it more readable.

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 8, 2018

Author Contributor

BTW, don't take my explanations as objections to make the changes. They are just additional information to inform the decision, but ultimately I trust that you are more familiar with the project and have a better feeling of how things work and affect it in the long term.
I other words don't hold back the bike-shedding 😄

counsel.el Outdated

(defun parse-user-input (string)
"Parse user input in STRING into a pair of two strings: (query extra-ag-args).
Where query is the search term and extra-ag-args are any additional switches."

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 8, 2018

Collaborator

"Parse", "user input", and "query" are too vague, and "extra-ag-args" is too specific. What this function is actually doing is splitting a command into its constitutent program and switches (see (elisp) Tips for Defining). Furthermore, the result is a dotted list (query . extra-ag-args), not a true list (query extra-ag-args).

Note also that the Elisp convention is to write metasyntactic variables like query and extra-ag-args in uppercase, and to fill (M-q) docstrings at 65 columns wherever possible; see (elisp) Documentation Tips.

As such, I propose something more along the lines of:

(defun counsel--split-command (command)
  "Split shell COMMAND into its program and switches.
Return pair of corresponding strings (PROGRAM . SWITCHES)."

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 8, 2018

Author Contributor

Good points and thanks for including the links.

The function is not separating the command from the switches tho:
It operates only on the arguments to a command (i.e.: ag) but the command itself is not part of the input. It splits this argument into a pair of switches and the search-term (or search-query or search-regexp).

The current order of the resulting pair probably leads to confusion:
Now it is (query . extra-ag-args), but it could be more clear if it followed the order in which the user would write them: (extra-ag-args . query) or based on your feedback: (switches . search-term)

How about?

(defun counsel-split-command-args (arguments)
  "Split ARGUMENTS into its switches and search term.
Return pair of corresponding strings (SWITCHES. SEARCH-TERM)."

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 8, 2018

Collaborator

SGTM, and thanks for the explanation. My only remaining bikesheds would be to use the prefix counsel-- instead of counsel-, as this is an internal function (see the previous link on Tips for Defining), and to suggest "needle" as a shorter alternative to "search-term".

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 9, 2018

Author Contributor

Done

counsel.el Outdated
(format counsel-ag-command
(concat extra-ag-args
term
file))))

This comment has been minimized.

Copy link
@basil-conto

basil-conto May 8, 2018

Collaborator

I think this can be simplified a bit:

(defun counsel--format-ag-command (extra-args needle)
  "Construct a complete `counsel-ag-command' as a string.
EXTRA-ARGS is a string of the additional arguments.
NEEDLE is the search string."
  (format counsel-ag-command
          (if (string-match " \\(--\\) " extra-args)
              (replace-match needle t t extra-args 1)
            (concat extra-args needle))))

100% guaranteed untested.

Either way, I think the function should be redesigned or at least documented better so that it is clear whether extra-ag-args/term should contain leading or trailing spaces etc., because at the moment, if there is no -- present, term just gets appended to extra-ag-args without a space in-between.

This comment has been minimized.

Copy link
@basil-conto

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 8, 2018

Author Contributor

Sorry, I won't be able to get back to this until I'm back home tonight.

This comment has been minimized.

Copy link
@DEADB17

DEADB17 May 9, 2018

Author Contributor

I tested your version and didn't find any issues. Thanks.
I also added an extra space when concatenating (concat extra-args " " needle) so that callers don't have to care about it.
I also tried trimming the arguments with string-trim before concating them but since white-space makes no difference when calling the command I decided to remove it in the end and avoid the extra work.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2018

@basil-conto I'd like to fix the issue withe the character count too:

The minimum character count in the ag-prompt is negative when switches are used.

I haven't had time to look into it yet. If you have any ideas let me know.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2018

The minimum character count in the ag-prompt is negative when switches are used.

Can you please elaborate, ideally with a reproducible recipe? I'm not sure what this means.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2018

The prompt requires at least 3 characters of input before it starts searching.
But when using switches the counter becomes negative:

screen shot 2018-05-10 at 1 45 44 pm

  • --ignore x --ignore y -- -22 chars more
  • --ignore x --ignore y -- v-23 chars more
  • --ignore x --ignore y -- va-24 chars more
  • --ignore x --ignore y -- vardoes the search

It's basically the length of the switches string - 3.

I imagine it should be an easy fix but I haven't had time to look around the code-base.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented May 14, 2018

@DEADB17 Is this still WIP? Or getting close to a mergable state:)?

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

The only remaining issue is the negative remaining characters above which I haven't had time to look into yet.
An alternative could be to merge the PR now and open a separate issue for that bug as it does not break functionality.

@d1egoaz

This comment has been minimized.

Copy link

commented May 30, 2018

thanks!! this looks awesome!

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

@abo-abo @basil-conto Are you OK with merging this as is?

  • The character count bug is cosmetic, it does not affect the functionality.
  • I don't have time to look into it at the moment.
  • If someone else is bothered by it they might be compelled to submit a patch 😉

If you are OK with the merge I can squash and rebase on master.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2018

@abo-abo @basil-conto Are you OK with merging this as is?

No need to ask for my approval, it's not important. Thanks though, and LGTM. :)

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch Jun 5, 2018

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

No need to ask for my approval

@basil-conto Your suggestions in the review were very useful. I asked in case you had something else to add.

I squashed the changes. This is ready for merging.

Regarding the C-cC-m recursive editing mentioned in this comment, I think that should be a separate PR.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

Regarding the C-cC-m recursive editing mentioned in this comment, I think that should be a separate PR.

I never suggested adding a recursive edit command in this PR. Rather, the discussion in #1408 made me realise that Counsel's search commands are quickly becoming very inconsistent, so I sought input from others regarding what type of consistency (i.e. UI) we would like to see.

The problem with inconsistency is that it gives rise to bugs and makes it increasingly harder for new contributors to come up to speed and be able to fix them.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

Yes, by separate PR I meant an independent exploration of alternatives.

The question is, whether we should merge this PR or wait until there is a recursive edit implementation, compare and then decide. My opinion is that we should merge this and then if we find that there is a better approach, remove this code when incorporating the alternative.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

The question is, whether we should merge this PR or wait until there is a recursive edit implementation, compare and then decide.

I don't see why we should wait. As I've said in #1408, the recursive edit implementation already exists, but is limited to counsel-git-grep in the form of counsel-git-grep-switch-cmd. This PR is adding a different UI to a different set of commands. There's no conflict, only gaps in functionality, and these can easily be filled in due course. There's a reason why I asked for general consensus on how to move forward in #1408 and not in this PR.

My opinion is that we should merge this and then if we find that there is a better approach, remove this code when incorporating the alternative.

I also think this can be merged and, as I've said in #1408, I can't imagine what a better approach would look like. If anyone had thought of a better approach, then you probably wouldn't have made this PR. :)

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 8, 2018

I'm having problems with getting the input --elisp -- require to work in this repo.

I think the problem originates in this code:

(setq counsel-ag-command
      (format counsel-ag-command (concat extra-ag-args " -- " "%s" file)))

which results in counsel-ag-command becoming "ag --nocolor --nogroup -- %s".

Afterwards, the command passed to the shell becomes:

ag --nocolor --nogroup  -- --elisp  require

which is wrong.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2018

@abo-abo That line is changed in the PR:
https://github.com/DEADB17/swiper/blob/0b422f0910ce907d36d2102fe8bea1302d3d2b20/counsel.el#L2309

It looks like you are running a different version. Could you re-evaluate ivy.el and counsel.el and try again?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2018

@DEADB17 Does the following "fix" the negative remaining characters issue?

diff --git a/counsel.el b/counsel.el
index 632ff31..a99d6b4 100644
--- a/counsel.el
+++ b/counsel.el
@@ -2275,7 +2275,8 @@ counsel-ag-function
     (let ((switches (car command-args))
           (search-term (cdr command-args)))
       (if (< (length search-term) 3)
-          (counsel-more-chars 3)
+          (let ((ivy-text search-term))
+            (counsel-more-chars 3))
         (let ((default-directory (ivy-state-directory ivy-last))
               (regex (counsel-unquote-regex-parens
                       (setq ivy--old-re

Good luck rebasing on top of 30 commits to counsel.el. :)

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2018

Good luck rebasing on top of 30 commits to counsel.el. :)

Same wishes from me :) Maybe making a new commit is faster.

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch Jun 19, 2018

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch Jun 19, 2018

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

I think it's ready to go.
Rebased and added the character count fix (Thanks @basil-conto !).

Tested on this repo, all returning correct results:

  • --elisp -- require using counsel-ag
  • --org -- require using counsel-ag
  • -g*.el -- require using counsel-rg
  • -g*.org -- require using counsel-rg
  • --glob '*.el' -- require using counsel-rg
  • --glob '*.org' -- require using counsel-rg
  • -g!*.el -- req\w\w\w\w\w\w using counsel-rg
  • -g!*.el -- require "+..* using counsel-rg

Of course more eyes on the code and more testing is welcome 😸 .
In the mean time I'm going to use this new version for work to test it a bit more.

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch Jun 19, 2018

@abo-abo abo-abo closed this Jun 19, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

Tested and merged, thanks all.

Small note, I'm not happy with counsel--command-args-separator-length, but it's easy to factor out later.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

@DEADB17 Oops, looking through the comments thread, I don't see anything about Emacs Copyright Assignment. Do you have one already?

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

@abo-abo

Oops, looking through the comments thread, I don't see anything about Emacs Copyright Assignment. Do you have one already?

I don't. What do I need to do?

I'm not happy with counsel--command-args-separator-length, but it's easy to factor out later.

Do you mean calculating the length on each call to counsel--split-command-args to avoid the constant?
My thinking was that since the package has lexical-binding it wouldn't pollute the globals.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

I don't. What do I need to do?

Please see https://github.com/abo-abo/swiper/blob/master/CONTRIBUTING.org#copyright-assignment for more info. I'll revert the merge and re-open the PR in the meantime.

Do you mean calculating the length on each call to counsel--split-command-args to avoid the constant?

Yes. I think the calculation effort is negligible compared to the risk of two variables going out of sync.

@abo-abo abo-abo reopened this Jun 19, 2018

@DEADB17 DEADB17 changed the title [WIP] Interactively specify switches Interactively specify switches Jun 20, 2018

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch Jun 20, 2018

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

I inlined counsel--split-command-args and sent the email to FSF.
I'll ping back once the paperwork is ready.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2018

Thanks.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2018

This PR also closes #1512, right?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2018

This PR also closes #1512, right?

As well as #1688.

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch 2 times, most recently Aug 4, 2018

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

I got the FSF signed copyright assignment letter and rebased the PR.
Next steps to get it merged?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2018

I got the FSF signed copyright assignment letter and rebased the PR.

Thanks.

Next steps to get it merged?

Can you please mention in the commit message that it also closes issues #1512 and #1688?

Interactively specify switches
closes #1408,
closes #1512,
closes #1688

@DEADB17 DEADB17 force-pushed the DEADB17:dash-dash-opts branch to cd5314d Aug 4, 2018

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

@abo-abo This should be ready for merging. I have the FSF paperwork sorted out. Let me know if there is anything else.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2018

@DEADB17 Thanks. Did you use your DEADB17@... email address when applying? Because I don't see it in the copyright.list file. So I suggest either you change the email address used in the commit message, or update the one used in the copyright.list file, they have to match.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@abo-abo Yes I did.
My document was singed in mid July and I received the copy 3 days ago.
I suspect they haven't updated the list yet. They are short on staff and taking longer than usual.
I cannot find the copyright.list file. Is there a public URL I can check?
Also, I just sent a request to verify the list to fsf-records@gnu.org as suggested here.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Aug 7, 2018

I cannot find the copyright.list file.

This is a file on the fencepost server, where all emails with CA are listed; it's mentioned in the link you gave.

Also, I just sent a request to verify the list to fsf-records@gnu.org as suggested here.

Thanks.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

@abo-abo Would you mind checking again?
Use all lowercase when searching for my username deadb17

I was not able to access the fence-post server.

@abo-abo abo-abo closed this in 826bb5e Aug 13, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Aug 13, 2018

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.