search

yuya373 / emacs-slack

slack client for emacs
1.11k stars 117 forks source link

add (helm "3.0") to Package-Requires #501

Closed gonewest818 closed 4 years ago

gonewest818 commented 4 years ago

Fix #499

It's not clear what version of helm is actually required, but since helm-slack.el was introduced in September 2018 and helm version 3.0 was released in August 2018, this seems like a reasonable choice.

Note that 99% of all the downloads of helm are from MELPA which continuously builds the latest commit from github (not MELPA stable which contains the version tagged releases). So it's likely many users are using versions of helm newer than 3.0 depending when they last updated their packages.

yuya373 commented 4 years ago

I want to make helm as optional package. Should I change (require helm) to (declare-function some-function "helm")?

gonewest818 commented 4 years ago

Here's a discussion on reddit that might be helpful: https://www.reddit.com/r/emacs/comments/4dlk68/what_is_best_way_to_create_optional_library/

I think the correct answer is (ironically, but so typical for the internet these days) the one that got rated 0 points at the bottom of the page: "I think conditional dependencies should be considered an anti-pattern. If you have core functionality that doesn't depend on helm, then publish that. If you have optional functionality that does depend on helm, publish it separately. Conditional dependencies just lead to confusion..."

So in other words if you want to make the helm dependency optional then it's least confusing if you just publish helm-slack as a distinct package.

Otherwise you're going to be doing stuff conditionally (I haven't actually tested this)

(if (featurep 'helm)             ; only if helm is present
  (with-no-warnings              ; suppress compiler errors for what follows

    ;; ... insert the entire contents of the file in here

  )
)
yuya373 commented 4 years ago

It was very helpful. Thank you for the advice. I created helm-slack as a separate package and remove dependency on helm from this package in https://github.com/yuya373/emacs-slack/pull/502

yuya373 commented 4 years ago

I've merged #502, so close this pull request. Thank you!