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

Add counsel-url-expand helper for ivy-ffap-url-functions #1164

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bcully
Contributor

bcully commented Aug 15, 2017

counsel-url-expand makes it easy to add keyword to URL expansions for ivy-ffap, simply by adding a (REGEXP . FORMAT) pair to counsel-url-expansions.

bcully added some commits Aug 15, 2017

Add counsel-url-expand helper for ivy-ffap-url-functions
counsel-url-expand makes it easy to add keyword to URL expansions for
ivy-ffap, simply by adding a (REGEXP . FORMAT) pair to
counsel-url-expansions.
@bcully

This comment has been minimized.

Contributor

bcully commented Aug 15, 2017

That's what I get for using emacs 25. I'll remove the dependency on seq.

counsel.el Outdated
@@ -1678,6 +1679,57 @@ When INITIAL-INPUT is non-nil, use it in the minibuffer during completion."
(format "http://debbugs.gnu.org/cgi/bugreport.cgi?bug=%s"
(substring url 1)))))))
(defvar counsel-url-expansions
nil
"List of (REGEXP . FORMAT) pairs.

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

I suggest a summary line slightly more descriptive and suggestive of the variable's purpose, e.g. "Map URL regexps to their expansion format string" or similar. The datatype structure can then be further described in the docstring body.

Another nitpick is that the variable's initial value, nil, could be placed on the same line as the variable's name.

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

ok

counsel.el Outdated
If the format element is a function, more powerful
transformations are possible. As an example,
'(\"^issue\\([0-9]+\\)$\" .

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

Nitpick: Why not give regexp examples with beginning and end of string, rather than line, anchors?

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

E.g.

\"\\`issue\\([[:digit:]]+\\)\\'\"

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

I guess I'm less familiar with emacs regular expressions. ^/$ are just more common and the distinction here seems a little academic. But I suppose ` and ' are more precise, so I'll adjust them.

This comment has been minimized.

@basil-conto

basil-conto Aug 31, 2017

Contributor

Hence my starting the comment with 'nitpick'. :) I'm just of the opinion that examples ought to be as precise/idiomatic as possible.

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

fair. As an elisp newbie I appreciate the attention you've given this PR.

counsel.el Outdated
(let ((result (funcall pred elt)))
(when result
(throw 'result result))))
nil))

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

You can just use cl-some instead of seq-some. FYI, global definitions should be prefixed with the package name by Elisp convention.

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

ok, thanks.

(let ((regexp (car pair))
(formatter (cdr pair)))
(when (string-match regexp word-at-point)
(if (functionp formatter)

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

Just curious - any reason to prefer functionp over fboundp here?

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

I'm not a frequent elisp writer, so I don't know the conventions. I preferred functionp because I know that formatter is bound here in the let, and I would tend to use fboundp when I don't know whether a symbol exists at all. But I recognize that we're talking about just the function slot of the symbol (or whatever the word is), so the distinction is hazy. If there's a reason or convention for preferring one over the other I'd be happy to follow it.

This comment has been minimized.

@basil-conto

basil-conto Aug 31, 2017

Contributor

Sorry, I should have looked into this more before asking. My habit is simply to ask the author first, because they may have already thought about it.

In older Emacsen, functionp used to return t for any kind of function, including things which can't be passed to funcall, such as macros and special forms. Somehow I also managed to conflate functionp with doing "more than what is usually necessary" in my mind.

Contrast that with the current description of functionp as returning t for any valid argument to funcall. In which case using fboundp instead of functionp here would simply be wrong.

counsel.el Outdated
"Expand word at point using `counsel-url-expansions'.
The first pair in the list whose regexp matches the word at point
will be expanded according to its format. This function is
intended to be used by `ivy-ffap-url-functions' to browse the

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

Nitpick: I think it reads better to replace "by" with one of "for", "with" or "in", as the object is a variable, not a function, and so does not directly "use" counsel-url-expand.

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

ack

ivy-test.el Outdated
(ert-deftest counsel-url-expand ()
"Test ffap expansion using counsel-url-expansions."
;; no expansions defined
(let ((counsel-url-expansions nil))

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

This comes down to a preference in style, but FYI this can also be written as (let (counsel-url-expansions) ...).

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

thanks, I'll switch.

ivy-test.el Outdated
"Test ffap expansion using counsel-url-expansions."
;; no expansions defined
(let ((counsel-url-expansions nil))
(should (eql (counsel-url-expand) nil)))

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

Nitpick: Why not just use eq?

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

yeah not sure why I used eql, will fix.

ivy-test.el Outdated
(concat "https://foo.com/issues/"
(match-string 1 word)))))))
;; no match
(defun current-word () "foobar")

This comment has been minimized.

@basil-conto

basil-conto Aug 30, 2017

Contributor

Probably not a wise idea to permanently redefine this function for the sake of a test. Why not write a convenience function/macro which performs the moral equivalent of

(with-temp-buffer
  (insert "foobar")
  (should (equal (counsel-url-expand) nil)))

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

I hadn't realized that definition would escape the test environment, but indeed it does. Will fix.

counsel.el Outdated
If the format element is a function, more powerful
transformations are possible. As an example,
'(\"\\'issue\\([[:digit:]]+\\)\\'\" .

This comment has been minimized.

@basil-conto

basil-conto Aug 31, 2017

Contributor

Thinko: First anchor should be backtick, not apostrophe.

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

oops.

ivy-test.el Outdated
`(with-temp-buffer
(insert ,text)
(goto-char (point-min))
,body))

This comment has been minimized.

@basil-conto

basil-conto Aug 31, 2017

Contributor

It is more common and general to define body as a &rest argument and then splice it into the backquoted form with ,@body.

Is moving to (point-min) necessary?

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

Thanks, I'll adjust it. The test passes without the goto-char call so I guess it isn't. I'd thought the point would be outside of the word after insert, but apparently not so.

Thanks again for the detailed review.

bcully added some commits Aug 31, 2017

counsel.el Outdated
@@ -1516,6 +1516,7 @@ TREE is the selected candidate."
(add-to-list 'ivy-ffap-url-functions 'counsel-github-url-p)
(add-to-list 'ivy-ffap-url-functions 'counsel-emacs-url-p)
(add-to-list 'ivy-ffap-url-functions 'counsel-url-expansions)

This comment has been minimized.

@abo-abo

abo-abo Aug 31, 2017

Owner

I think you want to use counsel-url-expand here?

This comment has been minimized.

@bcully

bcully Aug 31, 2017

Contributor

Ugh, yes. Must have been running stale code locally...

PR#1164: add counsel-url-expand to ivy-ffap-url-functions
Not the counsel-url-expansions variable that controls it.

@abo-abo abo-abo closed this in 01e305c Sep 11, 2017

abo-abo added a commit that referenced this pull request Sep 11, 2017

Minor changes to counsel-url-expand
* counsel.el: Rename `counsel-url-expansions' to `counsel-url-expansions-alist'.

* ivy-test.el (counsel-url-expand): Flip `ivy--string-buffer' and `equal'. Easier to eval just the `ivy--string-buffer'
  expression and get a string.

Re #1164
@abo-abo

This comment has been minimized.

Owner

abo-abo commented Sep 11, 2017

Thanks for the contribution.
Sorry for the delays in the review :)
Please review my linked changes on top of your code.
And please consider adding an entry to ivy.org.

@bcully

This comment has been minimized.

Contributor

bcully commented Sep 11, 2017

Thank you for merging. The additional changes look good to me, but I'm an elisp amateur :)
I'll take a look at ivy.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment