You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi! I'm one of the maintainers of original non-empty-containers, and I'm very impressed with the breadth of your library's API. non-empty-containers should most likely be deprecated in favour of this.
I recently needed partition behaviour for a non-empty Set, and noticed that that exact function exists here. So I pulled the dependency, and to my surprise, lens started being compiled as a dep. Surprised, I looked deeper, and noticed that these pulls in a vast web of dependencies, too many for the project I'm working on. I noticed also that these advertises the data-or library as a light-weight alternative.
So, this PR replaces usage of these in this library with data-or. You use the These type strictly for its constructor behaviour, not for any of the fanciness that its library's large dependency graph offers, so this was a straight-forward change.
Here is the dependency graph of nonempty-containers with these:
... and here it is with data-or:
I am also unhappy with all of lens being pulled in, and I too consider this to be pretty excessive. data-or seems to be a good substitute.
One potential downside to data-or over these is that Or does not have a Semigroup instance, which is pretty central to the design/usage of this library. But users who need an Semigroup instance will be able to define either an orphan instance, or a newtype wrapper with a Semigroup instance, or even pull in these if they don't care about the lens dependency.
Overall, LGTM. Thank you so much again, I really appreciate it!
My own concern was that data-or hasn't been updated in a while. Perhaps we could patch it as well, or barring that, try to convince the these author to create a these-core that exposes only the core data type and none of the fanciness, and then have nonempty-containers move back to that. these also has more "market share" in general.
I don't think, from an end-user standpoint, the choice of data-or or these should be of major consequence, since it's simple enough to write a one-off converter, I feel. It would be nice (but not particularly urgent, though) if data-or had Semigroup, Bifunctor, Functor, Traversable, MonadChronicle, etc. instances, though, and it probably would be worth sending over a PR at some point eventually maybe :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi! I'm one of the maintainers of original
non-empty-containers, and I'm very impressed with the breadth of your library's API.non-empty-containersshould most likely be deprecated in favour of this.I recently needed
partitionbehaviour for a non-emptySet, and noticed that that exact function exists here. So I pulled the dependency, and to my surprise,lensstarted being compiled as a dep. Surprised, I looked deeper, and noticed thatthesepulls in a vast web of dependencies, too many for the project I'm working on. I noticed also thattheseadvertises thedata-orlibrary as a light-weight alternative.So, this PR replaces usage of
thesein this library withdata-or. You use theThesetype strictly for its constructor behaviour, not for any of the fanciness that its library's large dependency graph offers, so this was a straight-forward change.Here is the dependency graph of
nonempty-containerswiththese:... and here it is with
data-or:Hopefully you find this useful.