vermiculus / magithub

**DEPRECATED - please use Forge instead!** -- Magit-based interfaces to GitHub
GNU General Public License v3.0
579 stars 63 forks source link

Replace soon-to-be-deprecated when-let #226

Closed aaronjensen closed 6 years ago

aaronjensen commented 6 years ago

when-let is deprecated in Emacs 26. Unfortunately, it is replaced by when-let* which does not exist in Emacs 25. This library was already using dash, so I replaced them with -when-let*.

Alternatively, something like this could be done, but I didn't think be a good idea to add that to every file.

vermiculus commented 6 years ago

Thanks! I wonder why they deprecated it, but thanks for catching :)

I'm also wondering if we can't just throw that one form into magithub-core.el:

(eval-when-compile
  (when (version< emacs-version "26")
    (defalias 'if-let* #'if-let)
    (defalias 'when-let* #'when-let)))

When compiling, do we not pull in required files? I'd think we would have to considering magithub-core already defines macros used elsewhere.

Would that be preferable than using more dash.el functions? (I mainly ask because I like to stick to Emacs standards -- and I haven't seen a strong reason to use dash functions as of yet.)

aaronjensen commented 6 years ago

Thanks! I wonder why they deprecated it, but thanks for catching :)

Sure thing. I think it may have been to get rid of the form of when-let that did not take a list.

I pushed up another commit that uses the alias for when-let. The catch is that though the code now uses when-let*, it will only behave like when-let* on Emacs 26. On <26 It will behave like when-let, so that's just something that all people modifying the code need to be aware of.

Another option might be to alias to the dash.el functions for <26?

Can you test this out on 25?

vermiculus commented 6 years ago

Seems to work like a charm when compiling, but not when interpreted. We should probably switch to eval-and-compile.

If you have no objections, I can do that and rebase at the same time.

aaronjensen commented 6 years ago

That works for me. Thanks!

vermiculus commented 6 years ago

Thanks!

aaronjensen commented 6 years ago

No problem, thanks for the merge.