ekmett / contravariant

Haskell 98 contravariant functors
http://hackage.haskell.org/package/contravariant
Other
73 stars 24 forks source link

Add Semigroup (Predicate a) instance #39

Closed phadej closed 6 years ago

phadej commented 6 years ago

Fixes #38

phadej commented 6 years ago

I copied the CPP conditional from other parts of the file, should I change them all into MIN_VERSION_base? Probably tagged&Proxy too?

Sent from my iPhone

On 7 Jan 2018, at 22.40, Ryan Scott notifications@github.com wrote:

@RyanGlScott requested changes on this pull request.

LGTM besides one quibble about CPP usage.

In src/Data/Functor/Contravariant.hs:

@@ -310,9 +310,18 @@ newtype Predicate a = Predicate { getPredicate :: a -> Bool } instance Contravariant Predicate where contramap f g = Predicate $ getPredicate g . f

+#if defined(MIN_VERSION_semigroups) || GLASGOW_HASKELL >= 711 Use MIN_VERSION_base(4,9,0) instead of GLASGOW_HASKELL >= 711.

In src/Data/Functor/Contravariant.hs:

instance Monoid (Predicate a) where mempty = Predicate $ const True +#if defined(MIN_VERSION_semigroups) || __GLASGOW_HASKELL__ >= 711 Same here.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

RyanGlScott commented 6 years ago

I copied the CPP conditional from other parts of the file

Ah, fair enough. I (personally) believe we should eschew this use of __GLASGOW_HASKELL__ to implicitly represent base bounds. If you feel motivated to convert the other sketchy uses of __GLASGOW_HASKELL__ as well, that would be nice, but if not, I can do it myself after merging.

phadej commented 6 years ago

Yes, let's use MIN_VERSION_base, it's more correct (as that's is really base issue).

Let's merge this, and do refactoring later.