-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Enum typeclass #3458
Enum typeclass #3458
Conversation
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.
There are breaking changes here, but I think they can be avoided without changing the core intent, and I like this addition.
I have one general question (in addition to one specific one about the members
method): what do you think about using something like Enumerable
instead of Enum
? I think reusing the Haskell terminology here might not be the right thing to do, given all the different things "enum" means in Scala, Dotty, and Java.
partialPrevious(a).getOrElse(maxBound) | ||
|
||
/** Enumerate the members in ascending order. */ | ||
def members: List[A] = { |
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.
Should this maybe be an Iterable
or LazyList
/ Stream
? Having a generic method that returns all members as a list in a context where instances will often be for types like Int
seems not really ideal.
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 agree! I toyed around with Iterable
, but didn't like the idea because it's not pure iirc, and the idea of having an impure return type irked me. I don't have a strong preference on either lazy types but I think a NonEmptyLazyList
sounds good? I can then add membersAscending
/membersDescending
on the LowerBounded
and UpperBounded
Enum
types.
@yilinwei Also, to answer your first question: the instances will need to go in all of the type class roots (see the discussion here), and since this is a rare change that adds new type class roots, it will be a little more complicated than usual. I'm happy to take a closer look at that part once the bincompat issues are resolved, if that works for you. |
Thanks for looking at this! I thought I had ran mima but evidently not correctly. I'll fix the mima issues and ask if I have any particular questions. Regarding the naming change, I think it makes sense; even more so because we've split it up into |
821e52e
to
5b3695b
Compare
127925c
to
6081197
Compare
Codecov Report
@@ Coverage Diff @@
## master #3458 +/- ##
==========================================
- Coverage 91.77% 91.56% -0.21%
==========================================
Files 383 386 +3
Lines 8400 8466 +66
Branches 208 236 +28
==========================================
+ Hits 7709 7752 +43
- Misses 691 714 +23 |
@travisbrown I've fixed the mima issues and addressed the comments; I seem to be having some dotty issues in the CI. EDIT. CI issues were fixed with a clear of the cache. |
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.
This looks good to me, thanks!
Add the
Enum
typeclass #2342 .A few comments:
UpperBounded
andLowerBounded
companion objects; should these be deleted?partialOrder
be the same field as theorder
just with the bound overridden?