commercialhaskell / rio

A standard library for Haskell
Other
838 stars 54 forks source link

Bi* Classes #188

Closed fosskers closed 4 years ago

fosskers commented 5 years ago

This PR exposes Bifunctor, Bifoldable, and Bitraversable, which are in base as of 4.10. Luckily, rio already enforces a >= 4.10 bound!

Note: The first and second functions from Control.Arrow, while the originals, can be considered special cases of those from Bifunctor. Specially, the Bifunctor variants work on both Tuple and Either, while those of Arrow work only on Tuple. So, this PR hides the Arrow variants.

Functions which have not been exposed:

Aliases

Pointless?

snoyberg commented 5 years ago

can be considered special cases of those from Bifunctor

I don't believe this is strictly true. The Arrow variants work on any arrow, the Bifunctor versions work on only functions. I agree that in 20-20 hindsight, the Bifunctor version is better, but this is technically a breaking change. Options:

  1. Don't include the change at all
  2. Include the change next time we have a major breaking change (which may be soon if desired)
  3. Decide that this is such an uninteresting breakage that we include it here anyway
akhra commented 5 years ago

I vote for #2 while registering thorough non-resistance to #3.

fosskers commented 5 years ago

Shoving all the Lens stuff into RIO.Lens (as in #189 ) would induce a breaking change too, so we might be able to shove these all in at once.

fosskers commented 5 years ago

As for Arrow-vs-Bifunctor, what would the average user expect out of say first? Would they expect the Arrow variant, or the Bifunctor? My bias is toward the latter, guessing that the intersection of people who use RIO and people who enjoy the Arrow abstraction (more than Bifunctor) is a small set.

Also: any thoughts on the excluded aliases?

snoyberg commented 5 years ago

I believe most people want the Bifunctor version, which is why I agree that it should be exported instead. I'd say let's split this into two PRs: one which continues with the Arrow versions of first and second, and a separate PR (maybe to a new next branch) that moves over to Bifunctor.

I expect that we'll want to include some breaking changes to the logging system in the 0.2 release, so I don't want to gate this PR on breaking changes.

fosskers commented 5 years ago

~Roger, I can rollback for the Arrow changes for now.~

Actually, then how should the first and second from Bifunctor be exposed (everything is re-exported from RIO.Prelude)? Just bimap for now, with the first/second switch "to come soon"?

fosskers commented 4 years ago

Updated to be fresh with master. Now to return to the versioning debate. @snoyberg is right that technically this is a breaking change. first and second from Bifunctor works over Tuple and Either, while those from Arrow work over Tuple and a -> b. The question is, how likely is it that a rio user out there is using the a -> b variant?

Thinking out loud:

My guess is that m is close to, if not, 0. So I'm in favour of Bifunctor "winning" this battle.

Now whether or not to bump the major version...

Thoughts?

snoyberg commented 4 years ago

Let's go for the change without a major version bump.

fosskers commented 4 years ago

Okay, that should do it.

snoyberg commented 4 years ago

Thanks!