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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add recipe for counsel-osx-app #3929

Merged
merged 1 commit into from May 30, 2016
Merged

add recipe for counsel-osx-app #3929

merged 1 commit into from May 30, 2016

Conversation

d12frosted
Copy link
Contributor

Simple as that 馃樃

@purcell
Copy link
Member

purcell commented May 25, 2016

Thanks! The prefix for public symbols should really be counsel-osx-app- throughout, not counsel-apps-, to match the package name and avoid possible future clashes with other packages.

@purcell
Copy link
Member

purcell commented May 25, 2016

Also, you're using user-error, which is unavailable in some of the Emacs versions which ivy supports, so you should explicitly depend on the minimum necessary Emacs version via your Package-Requires clause.

@purcell
Copy link
Member

purcell commented May 25, 2016

Bonus note:

(if (bound-and-true-p file)
    (format "open '%s' -a '%s'" file app)
  (format "open '%s'" app))

will break if filenames have quotes in them. Just use (format "open %s -a %s" (shell-quote-argument file) (shell-quote-argument app)) instead.

@d12frosted
Copy link
Contributor Author

Thanks! The prefix for public symbols should really be counsel-osx-app- throughout, not counsel-apps-, to match the package name and avoid possible future clashes with other packages.

Wow. I clearly forgot to do that after I renamed the package. Thanks for catching this 馃槉

Also, you're using user-error, which is unavailable in some of the Emacs versions which ivy supports, so you should explicitly depend on the minimum necessary Emacs version via your Package-Requires clause.

Ah, I thought that it's enough to require ivy, so proper version of Emacs will be required by cross dependency. But it's not a big deal.

Bonus note:

Ah, I didn't know. Thanks for that. It's time to update some other functions I have in my init.el 馃樃

@purcell
Copy link
Member

purcell commented May 25, 2016

Ah, I thought that it's enough to require ivy, so proper version of Emacs will be required by cross dependency. But it's not a big deal.

Ivy needs Emacs 24.1, but I think user-error was added a little later. I might be wrong, though. Good to be explicit with dependencies in any case.

@xuchunyang
Copy link
Contributor

Ivy needs Emacs 24.1, but I think user-error was added a little later

user-error was added in Emacs 24.3: https://github.com/emacs-mirror/emacs/blob/1ee91bf89176251f6e399c8436dca0248cdd6f6b/etc/NEWS.24#L2207

d12frosted added a commit to d12frosted/counsel-osx-app that referenced this pull request May 25, 2016
Because `user-error` was added in `24.3`.

melpa/melpa#3929 (comment)
@d12frosted
Copy link
Contributor Author

@xuchunyang thanks for finding the proper version number 馃挴

Updated my package.

@d12frosted
Copy link
Contributor Author

d12frosted commented May 27, 2016

Ping 馃樃

Is there anything I should do?

@purcell purcell merged commit 926d0ab into melpa:master May 30, 2016
@purcell
Copy link
Member

purcell commented May 30, 2016

Thanks. Sorry for the slow response. Merged now. :-)

@d12frosted d12frosted deleted the recipe/counsel-osx-app branch May 30, 2016 03:37
@d12frosted
Copy link
Contributor Author

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