search

magnars / dash.el

A modern list library for Emacs
GNU General Public License v3.0
1.67k stars 137 forks source link

Absorb dash-functional into dash #356

Closed basil-conto closed 3 years ago

basil-conto commented 3 years ago

I'm currently getting things prepared for releasing Dash 2.18.0, whose main goal is to get copyright and other cleanups sorted in order to update the very stale version of Dash currently on GNU ELPA.

In doing so, I noticed that GNU ELPA currently bundles the entire Dash repo, including admin files and the kitchen sink. By contrast, MELPA bundles dash.el and dash-functional.el separately.

For backward compatibility, the status quo must be maintained on GNU ELPA.

I was wondering whether MELPA could be brought into line with GNU ELPA by bundling both files together under the dash package in a backward compatible way? Or must/will they continue to be bundled separately?

Thanks.

Cc: @purcell, @tarsius

P.S. I wasn't sure whether to open this issue here or on the MELPA tracker; if the latter is preferred I'll happily oblige.

basil-conto commented 3 years ago

BTW, my understanding from skimming #45 is that dash-functional.el was first split from dash.el because of the former's dependency on Emacs 24.

Now that dash.el requires Emacs 24, could we and would we want to merge the two files again?

I'm guessing this could be done on MELPA in a backward-compatible way by keeping the last released version of dash-functional.el on its own branch, with some obsoletion warning or other? Then the repo's main branch could maintain a single, joint dash.el.

What do people think? Am I missing something?

tarsius commented 3 years ago

In doing so, I noticed that GNU ELPA currently bundles the entire Dash repo, including admin files and the kitchen sink. By contrast, MELPA bundles dash.el and dash-functional.el separately.

That's two separate issues.

  1. Stefan likes to keep the kitchen sink but does not object if the package maintainer decides otherwise. IMO you should exclude everything but *.el, LICENSE, README.md and dash.texi. Last I checked *.info got automatically removed, but this is still subject to change and you should double check your ignore rules using make dash.tar.
  2. Whether to keep dash-functional as a separate package and if not, how to transition.

Now that dash.el requires Emacs 24, could we and would we want to merge the two files again?

That was the plan I believe. I am strongly in favor of absorbing it.

I'm guessing this could be done on MELPA in a backward-compatible way by keeping the last released version of dash-functional.el on its own branch, with some obsoletion warning or other? Then the repo's main branch could maintain a single, joint dash.el.

Indeed, in this case we should probably go the extra mile and provide a transitional package.

basil-conto commented 3 years ago

That's two separate issues.

Right.

1.

I assumed your position was the standard de facto, but I'll revisit this (and maybe ping Stefan for comment) when we're ready to push to GNU ELPA, which is still a ways away.

  1. Whether to keep dash-functional as a separate package and if not, how to transition.

    Now that dash.el requires Emacs 24, could we and would we want to merge the two files again?

That was the plan I believe. I am strongly in favor of absorbing it. [...] Indeed, in this case we should probably go the extra mile and provide a transitional package.

Great. Let's see if the others are in favour of this as well.

basil-conto commented 3 years ago

I completely forgot we previously discussed some of this in #130. :)

Hopefully people's stances haven't changed since then.

magnars commented 3 years ago

Thumbs up from me.

fre. 1. jan. 2021 kl. 18:43 skrev Basil L. Contovounesios < notifications@github.com>:

That's two separate issues.

Right.

1.

I assumed your position was the standard de facto, but I'll revisit this (and maybe ping Stefan for comment) when we're ready to push to GNU ELPA, which is still a ways away.

  1. Whether to keep dash-functional as a separate package and if not, how to transition.

Now that dash.el requires Emacs 24, could we and would we want to merge the two files again?

That was the plan I believe. I am strongly in favor of absorbing it. [...] Indeed, in this case we should probably go the extra mile and provide a transitional package.

Great. Let's see if the others are in favour of this as well.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/magnars/dash.el/issues/356#issuecomment-753351053, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OL2ZLLHJGMJBT3CDG3SXYCVNANCNFSM4VQKSQTA .

purcell commented 3 years ago

Yeah, a transitional package would be important, because dash-functional has a ton of reverse dependencies. https://melpa.org/#/dash-functional

Generally in favour of recombining the two though.

Fuco1 commented 3 years ago

What is a transitional package? Would we make dash-functional simply require dash?

basil-conto commented 3 years ago

What is a transitional package?

I don't know whether MELPA has such a thing, but I was thinking of just changing the MELPA recipe for dash-functional to fetch from a compat/dash-functional branch in this repo, which will maintain the current (bumped) version of dash-functional.el as its own package for backward compatibility. I've already started this in commit 5f0f571497ec8b370e9582cd7165bd6abdb56abc on the aforementioned compat branch, if you want to take a look.

Would we make dash-functional simply require dash?

Here's what I currently envision:

The compat/transitional dash-functional.el would be unchanged, bar for the bumped version header and a deprecation notice in the package commentary.

The version of dash-functional.el on the trunk would contain nothing more than (require 'dash), a call to lwarn which outputs the same "this package is deprecated" warning as built-in Emacs packages under the lisp/obsolete/ tree currently emit, and a (provide 'dash-functional).

Meanwhile, the super-version of dash.el on the trunk would provide both dash and dash-functional named features.

I've already started this work on the WIP branch release/2.18.0, if you want to have a look.

Be warned: I plan to force-push to the aforementioned branches as I work on them. :)

tarsius commented 3 years ago

What is a transitional package? Would we make dash-functional simply require dash?

If by "simply" you mean "do that and and only that", then yes. (Though it can also show a warning.)

tarsius commented 3 years ago

I.e. dash-functional.el should be changed to be just:

;;; dash-functional.el --- Collection of useful combinators for Emacs Lisp  -*- lexical-binding: t -*-

;; Copyright (C) 2013-2021 Free Software Foundation, Inc.

;; Authors: Matus Goljer <matus.goljer@gmail.com>
;;          Magnar Sveen <magnars@gmail.com>
;; Package-Requires: ((dash "TODO(a)") (emacs "24"))
;; Keywords: extensions, lisp

;; This program is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; This program is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with this program.  If not, see <https://www.gnu.org/licenses/>.

;;; Commentary:

;; This package has been aborbed by the `dash' package.

;; If you are the author of a package that depends on `dash-functional',
;; then you should change that to depend on `dash' instead.

;; If you are the user of one or more packages that depend on
;; `dash-functional', then you will have to wait until the maintainers
;; of all of these packages have taken action before you can remove
;; `dash-functional'.

;; For discussion see TODO(b) link.

;;; Code:

(require 'dash)

(provide 'dash-functional)

;;; dash-functional.el ends here

Note that this contains two TODOs. (a) should first be replaced with the correct version, probably 2.18.0, then that should be committed and tagged with that same version string, so that Melpa-Stable picks it up. Then the version should be changed to 202101xx.xxxx for the latest Melpa-Unstable build of dash. Then that should be committed. (b) should be to a place where confused users find some documentation and a helping hand.

basil-conto commented 3 years ago

(b) should be to a place where confused users find some documentation and a helping hand.

How's the following for a start? Please suggest improvements.

https://github.com/magnars/dash.el/wiki/Obsoletion-of-dash-functional.el

tarsius commented 3 years ago

Since MELPA has historically distributed dash-functional as a separate package, with its own set of dependent packages, a transitional but deprecated dash-functional package must continue to exist for backward compatibility.

This will be achieved by freezing an upcoming dash-functional version 1.3.0 and continuing to serve that. This transitional package will cease to be maintained, and its commentary will advise switching to dash version 2.18.0 or later instead.

If we find a bug in, say, -fixfn and we only fix it in dash.el but not the frozen dash-functional.el 1.3.0, then it becomes unclear whether the new fixed version or the old unpatched version ends up being loaded. We could avoid that by making sure we don't freeze the backward-compatiblity dash-functional.el and don't forget to always update both versions.

Or we do what I suggested above. That seems like the much safer approach to me. What are your reasons for not doing it like that?

basil-conto commented 3 years ago

If we find a bug in, say, -fixfn and we only fix it in dash.el but not the frozen dash-functional.el 1.3.0, then it becomes unclear whether the new fixed version or the old unpatched version ends up being loaded. We could avoid that by making sure we don't freeze the backward-compatiblity dash-functional.el and don't forget to always update both versions.

Since the new super-dash will provide both features dash and dash-functional, and since dash-functional requires dash, I think it is safe to assume that anyone tracking dash or dash-functional updates will see the fixed version.

In the same way that Emacs 26 will never again see another update, I think we can freeze dash-functional indefinitely, if for no other reason than to encourage downstream packages to transition away from it.

The only reason it will still be provided as a separate package is to not break setups/packages that explicitly depend on it. For all other intensive purposes, it's a discontinued and superseded package, and any "bugs" present in its use should be fixed precisely by no longer using it, and tracking only dash instead.

Am I missing something?

Or we do what I suggested above. That seems like the much safer approach to me. What are your reasons for not doing it like that?

What is your suggestion? I thought I was already following it, but maybe I misunderstood.

Now that there's a wiki for this transition, my next steps were going to be:

  1. Update the dash-functional recipe on MELPA to track the compat/dash-functional branch, in which the commentary of dash-functional already warns of its impending obsoletion and points to the wiki
  2. Absorb dash-functional into dash on master

Then, when we're ready for the 2.18.0 release (blocked by a CA in #344):

  1. Bump dash on master to 2.18.0
  2. Bump dash-functional on compat/dash-functional to 1.3.0 and its dependency on dash to version 2.18.0

Do you see any issues with this?

Thanks.

basil-conto commented 3 years ago

To be clear, when I say "absorb", I mean the following:

tarsius commented 3 years ago 
* A `(provide 'dash-functional)` is added to `dash.el`

Even if you don't do that then loading dash-functional will load dash, which will define the functions previously defined in dash-functional. We want packages to transition to "load dash to get all of dash (including what used to be dash-functional)". If dash starts to provide dash-functional, then some maintainers might start dropping dash-functional as a Package-Require but keep it as a require.

Maybe once we remove dash-functional from the Elpas, then we could add (provide 'dash-functional) to dash.el, for the benefit of those who ignored the warning for a long time. Doing it at the beginning of the transitional period would likely prolong it.

* The contents of `dash-functional.el` become:
  ```emacs-lisp
  (require 'dash)
  (lwarn 'dash :warning ...)
  (provide 'dash-functional)
  ```

That's not what I got from the description on the wiki. To me that sounded like you were planning to copy code, not move it.

basil-conto commented 3 years ago

A (provide 'dash-functional) is added to dash.el

Even if you don't do that then loading dash-functional will load dash, which will define the functions previously defined in dash-functional.

The problem with this is that people might have both super-dash and frozen-dash-functional installed, in which case loading dash-functional.el will redefine all its definitions, which should be avoided.

We want packages to transition to "load dash to get all of dash (including what used to be dash-functional)".

Yes.

If dash starts to provide dash-functional, then some maintainers might start dropping dash-functional as a Package-Require but keep it as a require.

It wouldn't make sense if they did that, and would be a bug on their part, as far as I'm concerned.

That's not what I got from the description on the wiki. To me that sounded like you were planning to copy code, not move it.

I've tried to make that a bit clearer now:

All the definitions in dash-functional will be moved (not copied) into dash, and will be replaced with a deprecation warning.

basil-conto commented 3 years ago

If dash starts to provide dash-functional, then some maintainers might start dropping dash-functional as a Package-Require but keep it as a require.

It wouldn't make sense if they did that, and would be a bug on their part, as far as I'm concerned.

So I've made that explicit in the wiki too:

Any calls to (require 'dash-functional) or similar should be removed from your package, and replaced with (require 'dash).

tarsius commented 3 years ago

A (provide 'dash-functional) is added to dash.el

Even if you don't do that then loading dash-functional will load dash, which will define the functions previously defined in dash-functional.

The problem with this is that people might have both super-dash and frozen-dash-functional installed, in which case loading dash-functional.el will redefine all its definitions, which should be avoided.

I don't understand this. If "frozen-dash-functional" does not define anything (because everything was moved to dash), then how can loading dash-functional cause that to re-define anything?

basil-conto commented 3 years ago

I don't understand this. If "frozen-dash-functional" does not define anything (because everything was moved to dash), then how can loading dash-functional cause that to re-define anything?

Currently, there is a single dash-functional.el on master at version 1.2.0. I'm thinking of branching off two versions of dash-functional.el, an "absorbed" and a "frozen" one:

  1. On master, all of the definitions in dash-functional.el are moved to dash.el. An almost empty file dash-functional.el is kept around with an obsoletion warning for backward compatibility with GNU ELPA (which bundles both libraries in the same package) and other exotic setups that expect a dash-functional.el file to exist. In the future, we may decide to finally delete this "absorbed" dash-functional.el.

  2. On compat/dash-functional, dash-functional.el as it currently stands is frozen with all its definitions in place. This is for backward compatibility with MELPA, for the benefit of users who e.g. have an older dash and a newer dash-functional installed, since dash-functional currently depends on only (dash "2.0.0"). Once dash version 2.18.0 is released, we can bump this "frozen" version of dash-functional to version 1.3.0 and mention in the commentary that this is the last release and the package is superseded by dash version 2.18.0. In the future, we may decide to bump the dependencies of this "frozen" dash-functional to (dash "2.18.0") and remove all its definitions, as will have already happened on master.

I thought this was the smoothest path to least breakage on all ELPAs. Do you think the extra caution in (2) is unnecessary?

basil-conto commented 3 years ago

BTW, (2) can also involve an obsoletion warning, to make the transition more visible and actionable.

magnars commented 3 years ago

If there are no breaking changes, why would we need to frozen version? As long as we require dash from the near-empty dash-functional?

basil-conto commented 3 years ago

If there are no breaking changes, why would we need to frozen version? As long as we require dash from the near-empty dash-functional?

Because there may be a nonempty window in which users have a near-empty dash-functional but old dash installed, but maybe I'm just being paranoid.

basil-conto commented 3 years ago

I must be overthinking this, so here's the alternative plan of action: simultaneously release dash version 2.18.0 and dash-functional version 1.3.0, where the latter depends on (dash "2.18.0") and emits an obsoletion warning.

Any issues arising are blamed as pilot errors. Does that sound acceptable?

magnars commented 3 years ago

I might be underthinking it, but that sounds acceptable to me.

fre. 5. feb. 2021 kl. 18:27 skrev Basil L. Contovounesios < notifications@github.com>:

I must be overthinking this, so here's the alternative plan of action: simultaneously release dash version 2.18.0 and dash-functional version 1.3.0, where the latter depends on (dash "2.18.0") and emits an obsoletion warning.

Any issues arising are blamed as pilot errors. Does that sound acceptable?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/magnars/dash.el/issues/356#issuecomment-774173180, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OIVH4RDJUNRQQLTGZDS5QS6JANCNFSM4VQKSQTA .

purcell commented 3 years ago

I must be overthinking this, so here's the alternative plan of action: simultaneously release dash version 2.18.0 and dash-functional version 1.3.0, where the latter depends on (dash "2.18.0") and emits an obsoletion warning.

The older MELPA snapshot version of dash would satisfy that dependency, so in theory you could have a situation where a user updates only dash-functional but not dash. In practice, I wouldn't worry about that, and MELPA will rebuild both packages almost simultaneously.

In other words, this sounds like a good plan to me.

basil-conto commented 3 years ago

Thanks everyone, I have now simplified the transition plan wiki accordingly: https://github.com/magnars/dash.el/wiki/Obsoletion-of-dash-functional.el

basil-conto commented 3 years ago

For emitting an obsoletion warning when dash-functional is used, is it realistically feasible for MELPA to install it under a subdirectory called obsolete as Stefan suggests here?

Alternatively, does the following dance at the top-level of dash-functional.el seem like the right thing?

(eval-and-compile
  (let ((msg "Package dash-functional is obsolete; use dash 2.18.0 instead"))
    (if (and noninteractive (fboundp 'byte-compile-warn))
        (byte-compile-warn msg)
      (message "%s" msg))))
basil-conto commented 3 years ago

I've put this up for review in #371. When I merge that within the next week, I'll simultaneously bump the package versions and tag the release.