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

Getting rid of Data.Monoid.DecidablyEmpty #44

Open
1 of 4 tasks
endgame opened this issue May 21, 2022 · 6 comments
Open
1 of 4 tasks

Getting rid of Data.Monoid.DecidablyEmpty #44

endgame opened this issue May 21, 2022 · 6 comments

Comments

@endgame
Copy link
Contributor

endgame commented May 21, 2022

There's a comment at the top of Data.Monoid.DecidablyEmpty which reads: -- TODO upstream somwhere else?

That "somewhere else" already exists: Data.Monoid.Null from monoid-subclasses.

I had a look at the instances it provides, and it is missing the following ones:

instance MonoidNull a => MonoidNull (Identity a)
instance MonoidNull a => MonoidNull (WrappedMonoid a)
instance (Ord a, Bounded a) => MonoidNull (Max a)
instance (Ord a, Bounded a) => MonoidNull (Min a)
instance MonoidNull (Proxy a)
instance MonoidNull a => MonoidNull (Const a b)
instance MonoidNull a => MonoidNull (Down a)
instance MonoidNull p => MonoidNull (Par 1 p)
instance MonoidNull (U1 p)
instance MonoidNull (f p) => MonoidNull (Rec1 f p)
instance MonoidNull (f p) => MonoidNull (M1 i c f p)
instance MonoidNull c => MonoidNull (K1 i c p)
instance (MonoidNull (f p), MonoidNull (g p)) => MonoidNull ((f :*: g) p)
instance (MonoidNull (f p), MonoidNull (g p)) => MonoidNull ((f :.: g) p)
instance (...) => MonoidNull (a, b, c, d, e)

-- Should go to dependent-map
instance GCompare k => MonoidNull (DMap k v)

This would solve #42 but is too much work to hold up GHC 9.2 support. Tasks, as I see them:

  • Add the missing instances to monoid-subclasses and release
  • Add the missing instance to dependent-map and release
  • Replace Data.Monoid.DecidablyEmpty.DecidablyEmpty with type alias, set isEmpty = Data.Monoid.Null.null, deprecate both, and release
  • Remove deprecated Data.Monoid.DecidablyEmpty.

None of this seems particularly controversial. We might want monoid-subclasses anyway because of #37. It's quite a light package, so depending on it from dependent-map shouldn't be a problem either.

@endgame
Copy link
Contributor Author

endgame commented May 21, 2022

@Ericson2314 Let me know if you're happy with this plan, and I will begin making PRs.

@Ericson2314
Copy link
Member

Ericson2314 commented Sep 15, 2022

@endgame this one I have more mixed feelings about because monoid-subclasses is such overkill for our use-case.

@Ericson2314
Copy link
Member

Also, if we we had more "non-empty" data types, like haskell/containers#616, we could do this in a more "type directed" way which would be more satisfying.

(If you have spare cycles, I think they would be more usefully spent getting haskell/containers#616 over the finish line. I hope the conflicts are not too bad! The only thing remaining before that I remember was dealing with some perf regressions, which I think is just a matter of tweaking the inlining.)

@endgame
Copy link
Contributor Author

endgame commented Jan 10, 2024

@endgame this one I have more mixed feelings about because monoid-subclasses is such overkill for our use-case.

It's not that big a package, and its library component has a pretty reasonable dependency footprint. Regardless, I opened blamario/monoid-subclasses#50 .

@ryantrinkle
Copy link
Member

@endgame This is awesome, thank you!

@endgame
Copy link
Contributor Author

endgame commented Sep 29, 2024

Since I got the github notification, I thought I'd do dependent-map while I had a couple of minutes.

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