emilypi / smash

Smash products, Wedge products, and other Pointed stuff
34 stars 11 forks source link

Type variable ordering for partition functions #26

Closed subttle closed 3 years ago

subttle commented 3 years ago

Hello again!

Thank you again for all your work on this great package! I've noticed in the latest version (smash-0.1.2) there's a slight change in the type signature from the version prior (smash-0.1.1.0) for the partitionWedges, partitionCans, and partitionSmashes functions.

In the earlier version:

partitionWedges :: forall f t a b. (Foldable t, Alternative f) => t (Wedge a b) -> (f a, f b)
partitionCans :: forall f t a b. (Foldable t, Alternative f) => t (Can a b) -> (f a, f b)
partitionSmashes :: forall f t a b. (Foldable t, Alternative f) => t (Smash a b) -> (f a, f b)

Because the f was specified first, it was arguably easier to use TypeApplications to specify f and let the t be inferred. For example, in GHCi, one could:

λ> (partitionWedges @ Maybe) [Here (), There ()]
(Just (),Just ())

vs. the newer type signatures:

partitionWedges :: Foldable t => Alternative f => t (Wedge a b) -> (f a, f b)
partitionCans :: Foldable t => Alternative f => t (Can a b) -> (f a, f b)
partitionSmashes :: Foldable t => Alternative f => t (Smash a b) -> (f a, f b)

For which one now has to specify an extra type application:

λ> (partitionWedges @ _ @ Maybe) [Here (), There ()]
(Just (),Just ())

So as it stands, the current code is:

partitionWedges
    :: Foldable t
    => Alternative f
    => t (Wedge a b) -> (f a, f b)

but I think the type signature would be more preferable with Foldable t and Alternative f flipped:

partitionWedges
    :: Alternative f
    => Foldable t
    => t (Wedge a b) -> (f a, f b)

And similarly for partitionCans and partitionSmashes. It is perhaps a minor inconvenience, so perhaps you prefer to leave it (or perhaps I'm missing a reason for the changed ordering), but I thought I'd bring it to your attention in case you'd like to restore the previously available convenience. I apologize for such a long post for such a little issue. Thank you for your time and consideration!

emilypi commented 3 years ago

I changed this to remove the unncessary -XRankNTypes requirement in the source, and must have forgotten about this little rearrangement. That said, I am reading the PVP spec and realize these changes probably warranted a major version bump, which I can do ASAP this week, and I agree with your assessment that the Alternative probably requires an application more often than otherwise.

If you want to get in your Equivalence work, i'm happy to release a patch Monday or Tuesday.

emilypi commented 3 years ago

Closed via #26

emilypi commented 3 years ago

thanks @subttle!