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

Add more instances. #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Revision history for `interval-functor`.

## `0.1.0.0` — Sunday, 13 of September 2020.

* Add instances `Show1`, `Eq1`, `Ord1`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should mention NFData.


## `0.0.0.0` — Saturday, 11 of July 2020.

* First version. Released on an unsuspecting world.
4 changes: 3 additions & 1 deletion interval-functor.cabal
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
cabal-version: 2.2

name: interval-functor
version: 0.0.0.0
version: 0.1.0.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this constitutes a major release; there’s no backwards-compat issue that I can see.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New instances being exported may conflict with orphan instances defined in user code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make the same claim for any added symbol—it could break a build if you’re importing it open in a module using a symbol of the same name from elsewhere.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other sorts of names are safe to add assuming the consumers follow the advised practice of importing qualified or with explicit import lists. On the contrary, the import of classes is not possible to opt out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the official guide for PVP says that added non-orphan instances are not considered a breaking change. These instances here are non-orphan. So you are right.

synopsis: Intervals of functors.
description: Closed intervals in spaces described by a functor.
homepage: https://github.com/robrix/interval-functor
Expand Down Expand Up @@ -50,6 +50,8 @@ library
build-depends:
, base >= 4.10 && < 5
, transformers ^>= 0.5.6.2
, generic-data ^>= 0.9
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d rather we define the instance ourselves than take on this dependency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright!

, deepseq ^>=1.4
hs-source-dirs: src

test-suite test
Expand Down
18 changes: 18 additions & 0 deletions src/Data/Functor/Interval.hs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,14 @@ module Data.Functor.Interval
) where

import Control.Applicative (liftA2)
import Control.DeepSeq (NFData)
import Control.Monad.Trans.Class
import Data.Coerce (coerce)
import Data.Fixed (mod')
import Data.Function (on)
import Data.Functor.Classes (Show1, liftShowsPrec, Eq1, liftEq, Ord1, liftCompare)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we need to list the imports explicitly here, but if we’re going to, let’s sort this alphabetically. I think I have stylish-haskell config in this repo, if that helps.

import Data.Semigroup
import Generic.Data (gliftShowsPrec)
import GHC.Generics (Generic, Generic1)

-- | @f@-dimensional intervals with coordinates in @a@.
Expand All @@ -78,6 +81,21 @@ instance Show (f a) => Show (Interval f a) where
showsPrec p i = showParen (p > 3) $ showsPrec 4 (inf i) . showString "..." . showsPrec 4 (sup i)
{-# INLINE showsPrec #-}

instance Show1 f => Show1 (Interval f) where
liftShowsPrec = gliftShowsPrec
Comment on lines +84 to +85
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to not wanting to take on the extra dependency, it strikes me as strange that we’d have different behaviour for Show1 and Show.


instance Eq1 f => Eq1 (Interval f) where
liftEq f (Interval u v) (Interval u' v') = case liftEq f u u' of
True -> liftEq f v v'
False -> False

instance Ord1 f => Ord1 (Interval f) where
liftCompare f (Interval u v) (Interval u' v') = case liftCompare f u u' of
EQ -> liftCompare f v v'
x -> x

instance NFData (f a) => NFData (Interval f a)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NFData1 exists too, FWIW. Let’s define both.


instance Applicative f => Applicative (Interval f) where
pure = point . pure
{-# INLINE pure #-}
Expand Down