Skip to content

Replace these with data-or #2

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

Merged
merged 1 commit into from
May 14, 2019
Merged

Conversation

fosskers
Copy link
Contributor

@fosskers fosskers commented May 14, 2019

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:
deps
... and here it is with data-or:
after

Hopefully you find this useful.

@mstksg
Copy link
Owner

mstksg commented May 14, 2019

Thank you for the sizable contribution! :)

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!

@mstksg mstksg merged commit 65a75ed into mstksg:master May 14, 2019
mstksg added a commit that referenced this pull request May 14, 2019
mstksg added a commit that referenced this pull request May 14, 2019
@fosskers fosskers deleted the colin/data-or branch May 14, 2019 13:58
@fosskers
Copy link
Contributor Author

fosskers commented May 14, 2019

Thanks for the merge!

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.

@fosskers
Copy link
Contributor Author

Thanks also for the quick Hackage release. I'll be using this right away.

@mstksg
Copy link
Owner

mstksg commented May 17, 2019

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 :)

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

Successfully merging this pull request may close these issues.

2 participants