purescript / purescript-prelude

The PureScript Prelude
BSD 3-Clause "New" or "Revised" License
162 stars 88 forks source link

Record instances of Show, Eq, Ord etc #154

Closed LiamGoodacre closed 6 years ago

LiamGoodacre commented 6 years ago

Once we have moved RowToList (etc) to Prim, we can define these instances for Record.

hdgarrood commented 6 years ago

:+1: I would also like to include semigroup, monoid, semiring, ring, and commutative ring instances, which behave similarly to the existing Tuple ones. I don’t think we can do EuclideanRing because we won’t have an integral domain - for instance, (a,0) * (0,b) = (0,0) so we have zero divisors.

LiamGoodacre commented 6 years ago

We should discuss whether Ord (Record _) (and possibly others) makes sense given the key sorting from RowToList.

@i-am-tom - For now, only implement those in which the ordering isn't a significant factor. Show (Record _) is fine though.

hdgarrood commented 6 years ago

Can you remind me what the key sorting is? As Phil pointed out elsewhere I think it might be best not to provide Ord, as people will probably assume that if they use a type synonym like

type Person = { name :: String, age :: Int }

then they might expect lexicographical ordering based on the order fields are listed in the type synonym.

LiamGoodacre commented 6 years ago

Yes, it's not (can't be) the order the keys appear in the source (because they can appear in different orders and are still the same type). We could still provide Ord but the keys will always be in sort order - because that's what we get from RowToList. Also the derive for Ord currently does this sorting too - so it would be consistent with that. However, Phil mentioned that he's not sure this is necessarily a good default for Record.

There could separately exist an newtype for Records that is indexed by the keys in a specific order.

hdgarrood commented 6 years ago

In that case, shall we leave Ord (Record r) out for now and come back to it later if it comes up again?

garyb commented 6 years ago

Closing as we have this in the 0.12 branch now :tada: