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

Lexical binding in dash-functional.el #45

Closed
purcell opened this issue Aug 16, 2013 · 22 comments
Closed

Lexical binding in dash-functional.el #45

purcell opened this issue Aug 16, 2013 · 22 comments

Comments

@purcell
Copy link

purcell commented Aug 16, 2013

This code uses lexical-binding, which is not available in Emacs < 24.

As a result, if it's included in the dash package, then the entire package should depend on (emacs "24"), which is probably undesirable since many other Emacs 23-compatible packages would then transitively depend on Emacs 24.

Possible resolutions:

  1. Use lexical-let instead if possible and drop reliance on lexical-binding
  2. Distribute dash-functional as a separate package which depends on dash and Emacs 24
@purcell
Copy link
Author

purcell commented Aug 16, 2013

I guess I'd lean towards option 2...

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

Hmm, this split is turning ever more problematic. Dash used to come with a few functions that required Emacs 24. Maybe that set of functions could be increased?

Either way, I had planned a 2.0 release for some backwards breaking changes. But I now realize that is a lot harder to do without release support in melpa. I see you've been working on it, so maybe the best course of action is to wait for that?

In which case E23 would be continued supported on 1.x, and E24 on 2.x.

Thoughts?

@purcell
Copy link
Author

purcell commented Aug 16, 2013

The compatibility issue is a nasty one. In most cases it shouldn't be much work to keep backwards compatibility with Emacs 23, but that's up to you. Once dash in MELPA is E24-only, then many other packages also will be. And even Marmalade users will always get the latest version available to them -- there's no way for Emacs 23 users to "pin" an old version of dash. The same would apply to MELPA's planned "release" package archive.

You could analyse ~/.emacs.d/elpa/archives/melpa/archive-contents to see how many packages transitively require dash. But in your position, for a broad utility library like this, I'd probably put in some extra work to retain Emacs 23 compatibility, at least for dash.el itself.

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

Yes, you are absolutely right. I'll fix this tonight.

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

Do you see a problem with me just removing the (require 'dash-functional) from dash.el - so that you have to require them separately, but can get them both as dash in the package manager?

@purcell
Copy link
Author

purcell commented Aug 16, 2013

Ah, is that what the problem is? Yes, the require should absolutely be the other way around, with dash-functional requiring dash.el, and no reference to dash-functional.el from dash.el. The current structure presumably results in dash-functional.el not even byte-compiling cleanly. Given the way the code is currently arranged, I'd think the contents of dash-functional.el should just have gone straight into dash.el (ignoring issues of Emacs major version compatiblity, of course).

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

Yes, the problem is the lexical binding - which is why it is in a separate file. But is that a decent way of keeping backwards compatibility with Emacs 23? Or will it still break when E23 is trying to compile dash-functional.el (even if it is never required later)

@purcell
Copy link
Author

purcell commented Aug 16, 2013

E23 would likely have problems compiling it. But as suggested, you could make dash-functional a separate package which requires E24, while keeping the dash core compatible with E23. Or just replace the lexical binding with lexical-let if feasible.

@magnars magnars mentioned this issue Aug 16, 2013
@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 16, 2013

lexical-let is a hack from cl and not even that good :P So how do we feel about putting in stuff from cl package?

If this is a problem, then how about the entire dash-functional.el code gets wrapped in (when emacs version >= 24 ...)? Then on emacs 23 it won't even try to be compiled, problem solved.

And yes, you're right that the require shouldn't be there. That was stupid on my part :/

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

@Fuco1 That sounds like it might work.

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 16, 2013

Anyway, the fact that those functions require lexical binding to work doesn't mean they won't compile. It just wouldn't work, that's all, no? So there shouldn't be any problems to begin with.


By the way if you want to support e23 maybe adding the tests on travis back would be a good idea :P

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

By the way if you want to support e23 maybe adding the tests on travis back would be a good idea 📦

Haha, I'm so chock full of fail these days. Somebody come take my computer away. :)

@Fuco1
Copy link
Collaborator

Fuco1 commented Aug 16, 2013

So I've just compiled dash-functional.el on emacs 23.2 without problems (granted, it doesn't work... but so we can just say that in the docstring), however, dash.el fails with

In --partition-by-header:
dash.el:588:28:Warning: reference to free variable `h,'
dash.el:599:33:Error: Symbol's value as variable is void: h\,

Personally, I don't see much point in supporting e23. It's year and a half old (or more?) and so what... updating is trivial anyway.

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

Excellent, thank you. I've fixed the h, locally, and will re-enable E23 on CI. Then I'll get to work on dash-functional, with warnings about Emacs 24.

@magnars
Copy link
Owner

magnars commented Aug 16, 2013

dash-functional is now an optional package for those who do not care about E23 support. I reversed the dependency. Tests pass in E23. I'm going for an optimistic close. Wish me luck!

@magnars magnars closed this as completed Aug 16, 2013
@purcell
Copy link
Author

purcell commented Aug 17, 2013

By "optional package", do you mean that dash-functional going to be a separate package in MELPA, with a proper Package-Requires specifying Emacs 24?

@purcell
Copy link
Author

purcell commented Aug 17, 2013

@Fuco1 As for whether or not to support Emacs 23, it's very easy for you to say there's no point, but dash is a core utility library used by a ton of other packages, so making it incompatible (or partly and unpredictably compatible) with Emacs 23 has the effect of removing Emacs 23 compatible for all those other packages.

@magnars
Copy link
Owner

magnars commented Aug 17, 2013

By "optional package", do you mean that dash-functional going to be a separate package in MELPA, with a proper Package-Requires specifying Emacs 24?

That hadn't occured to me as important. I just figured we could mark some functions as requiring Emacs 24 like before. But I want to do what's right. What are the advantages of doing it like that?

As for melpa, would it be okay to serve both files from this repository, but without a -pkg file and instead two receipes pointing at the two files?

@purcell
Copy link
Author

purcell commented Aug 17, 2013

Yes. I personally think you should strive for clarity. Mixing functions which work in Emacs 23+ with functions which only work in Emacs 24 is a recipe for trouble. So yes, I'd aim for the following situation:

  • One repo, containing both .el files, and no -pkg.el
  • dash-functional.el has a Package-Requires depending on Emacs 24 and dash
  • dash.el is 100% usable in Emacs 23, and specifies no dependency on Emacs 24
  • MELPA has 2 recipes, and therefore 2 separate packages.

@magnars
Copy link
Owner

magnars commented Aug 18, 2013

melpa/melpa#963

@purcell
Copy link
Author

purcell commented Aug 18, 2013

Cool, recipe changes merged. Thanks!

@purcell purcell closed this as completed Aug 18, 2013
@magnars
Copy link
Owner

magnars commented Aug 18, 2013

And thank you for some sound advice.

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

No branches or pull requests

3 participants