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

Split {Functor,Foldable,Traversable}WithIndex to a separate package #941

Closed
arybczak opened this issue Sep 28, 2020 · 9 comments
Closed

Comments

@arybczak
Copy link

Mirror for well-typed/optics#309

optics also defines these classes and it would be good to not duplicate them.

Also, if it was a standalone package, people could use it without depending on either lens or optics.

@RyanGlScott
Copy link
Collaborator

RyanGlScott commented Sep 28, 2020

I'm unclear on what the relationship between the existing keys library and what you're looking for would be. If we do split out the *WithIndex classes, I'd like there to be some kind of story about classifying the purposes of these various libraries that provide keyed/indexed abstractions.

@phadej
Copy link
Collaborator

phadej commented Sep 28, 2020

keys description:

In practice this package is largely subsumed by the lens package, but it is maintained for now as it has much simpler dependencies.


keys is TypeFamilies and *WithIndex classes are FunctionalDependencies based.


keys has ZipWithKey class, but I use https://hackage.haskell.org/package/semialign-indexed-1.1/docs/Data-Semialign-Indexed.html nowadays (and there is a sibling packages defining classes on top of optics versions).

I'd be very happy to make semialign-indexed depend on something shared by lens and optics.


keys also has FoldableWithKey1 and TraversableWithKey1, but there are no analogous classes in lens or optics, so I'd say they could just be dropped. Foldable1 doesn't seem to take off.


lens variants have lens specific (conjoined) method. As there is no "proof" whether that is strictly needed and it does ties to make optimized do something which it might just decide to not do, I don't think they are important.

I guess @ekmett my disagree (but I'd leave the proof obligation on him, or whoever objects) to show they are strictly required, and optimizer cannot be made to do the right thing otherwise.


TL;DR keys vs new-package is type families or functional dependencies question.


The benefit of FD, is e.g. Each in optics. It is cleaner to add i with FDs, then have a type-family EachIndex or EachKey.

@RyanGlScott
Copy link
Collaborator

TL;DR keys vs new-package is type families or functional dependencies question.

I see.

lens variants have lens specific (conjoined) method. As there is no "proof" whether that is strictly needed and it does ties to make optimized do something which it might just decide to not do, I don't think they are important.

Yes, this is definitely an awkward sticking point. I suspect that if you tried factoring out everything that conjoined uses into its own package, you'd end up needing a surprising amount of machinery from lens. I'm unclear on what the consequences of removing the lens-specific methods from the *WithIndex classes would be.

@arybczak
Copy link
Author

arybczak commented Oct 4, 2020

TraversableWithIndex as itraversed, FoldableWithIndex has ifolded and FunctorWithIndex has imapped as a member, but their default implementations are never redefined (apart from itraversed being defined as traversed for [], ZipList and vector, though default itraversed uses itraverse, which then calls traversed, so that redefinition seems redundant).

FWIW optics doesn't define these as members and benchmarks comparing performance of itraversed etc. with lens show that there is no penalty for that.

@ekmett
Copy link
Owner

ekmett commented Oct 5, 2020 via email

@arybczak
Copy link
Author

arybczak commented Oct 5, 2020

I don’t know what you mean by there being no proof that the conjoined machinery is necessary

Ah, but we're not saying that (indeed, conjoined is quite nice).

It's about having itraversed/ifolded/imapped as standalone functions instead of class members.

@ekmett
Copy link
Owner

ekmett commented Oct 5, 2020 via email

@phadej
Copy link
Collaborator

phadej commented Oct 5, 2020

I presume moving them to top level definitions or definitions per package that needs a different presentation would make it easier for, say, optics to roll its own flavor of them then?

optics does roll own flavor on top of general FunctorWithIndex ...

This issue is a motivation to having just single FunctorWithIndex which would make it easier to folks to define it, than to depend on both lens and optics to define it. (Though, for Ix and At that still will be the case, have to start somewhere).

I’m happy to consider grossly deprecating all of keys except for this part of the API and translocating these classes these wholesale with the tweak. I’d also consider a fresh package as a destination rather than deal with causing existing users of the long deprecated-in-my-mind keys package undue pain.

I'm in favor of new package, say indexes.

Control.Lens.Indexed imports modules from array, (formerly) transformers, unordered-containers and vector. It's another small hill, but I do think that unordered-containers and vector maintainers could be persuaded to flip the dependency in the long term. indexes can IMO depend on all of these in the beginning. (This will cause problem to dependency-super-minimality of optics-core, but if that's the case, then we have a stalemate situation).

I don't think anything else have to be flipped, people who define FunctorWithIndex.. now by importing Control.Lens could still continue doing that, if lens decides to re-export them.

In other words, I don't see a need for a "package full of orphans".

@RyanGlScott
Copy link
Collaborator

This was done in #947 and #951.

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

4 participants