-
Notifications
You must be signed in to change notification settings - Fork 26
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 NonEmptySet
#7
Conversation
src/Data/Set/NonEmpty.purs
Outdated
unions :: forall f a. Foldable1 f => Ord a => f (NonEmptySet a) -> NonEmptySet a | ||
unions = foldl append (NonEmptySet Set.empty) | ||
|
||
-- | Form the set difference. `Nothing` if the sets are identical. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difference is lhs - rhs
, so I think it should be:
Form the set difference.
Nothing
if the first is a subset of the second.
src/Data/Set/NonEmpty.purs
Outdated
|
||
-- | Form the union of a non-empty collection of non-empty sets. | ||
unions :: forall f a. Foldable1 f => Ord a => f (NonEmptySet a) -> NonEmptySet a | ||
unions = foldl append (NonEmptySet Set.empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not unions = fold1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking. Another one I thought of earlier today is that we can have a Bounded
instance rather than providing findMin
and findMax
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow the Bounded
idea. Although Bounded a => Bounded (NonEmptySet a)
makes sense👍 . I don't see how that relates to findMin
, which is "give me the minimum element in a specific non empty set".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, Bounded a => Bounded (NonEmptySet a)
also doesn't make sense. Not sure what I was thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't know what I was thinking either 😁
@LiamGoodacre how's this look now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
No description provided.