-
Notifications
You must be signed in to change notification settings - Fork 50
Add Eq1 Ord1 to NonEmptyList LazyNonEmptyList #188
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
Conversation
CI shows that the Lazy version of this change fails to compile, but the non-Lazy version works fine. Don't know why. |
@@ -211,6 +211,12 @@ derive instance newtypeNonEmptyList :: Newtype (NonEmptyList a) _ | |||
derive newtype instance eqNonEmptyList :: Eq a => Eq (NonEmptyList a) | |||
derive newtype instance ordNonEmptyList :: Ord a => Ord (NonEmptyList a) | |||
|
|||
instance eq1NonEmptyList :: Eq1 NonEmptyList where | |||
eq1 (NonEmptyList lhs) (NonEmptyList rhs) = eq1 lhs rhs |
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.
Was hoping this shorthand would work, but it does not:
eq1 = over NonEmptyList eq1
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've seen a number of cases where omitting arguments in an instance doesn't work. But I think it's arguably better to include the arguments explicitly in any case.
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'd argue that the shorthand version is better, since it protects you from making a mistake like this:
compare1 (NonEmptyList rhs) (NonEmptyList lhs) = compare1 lhs rhs
Should these changes be tested, and if so, I guess I should also add the same tests to the empty-able lists 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 don’t think these need tests - the type system helps us enough here (via parametricity) that I think we can be confident these are doing the right thing without needing additional tests.
Actually let’s include the changelog updates in each PR please; the merge conflicts that would be introduced will all be very easy to fix. |
Thank you! |
Description of the change
Adds
Eq1
andOrd1
instances toNonEmptyList
andLazyNonEmptyList
Fixes #186
Checklist: