-
-
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
Enrich return type for Representable.apply and add doctests #2426
Conversation
The `Representable.apply` method was discarding information about its return type in a way that made it not very helpful for some use-cases. I believe that the change that I made to it makes it strictly more useful in a compatible way, but let me know if that doesn't seem to be the case. While creating this I noticed that our Representable instance for Tuple2 uses true for the first index and false for the second index. I'm not familiar with any precedents in this area, but I would have expected false = 0 index and true = 1 index. I assume that this is intended? I also added several scaladoc examples, because why not?!
@eli-jordan would you be willing to take a look at this and let us know your thoughts? |
Codecov Report
@@ Coverage Diff @@
## master #2426 +/- ##
=======================================
Coverage 95.22% 95.22%
=======================================
Files 351 351
Lines 6368 6368
Branches 279 276 -3
=======================================
Hits 6064 6064
Misses 304 304
Continue to review full report at Codecov.
|
@ceedubs The change looks good to me. Thanks for adding the doc tests, I probably should have done that originally :-/ Regarding the instance for |
Thanks for the prompt response @eli-jordan. As you said, ultimately the slot assignments for |
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.
Thanks!
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.
LGTM, Thanks! :)
The
Representable.apply
method was discarding information about itsreturn type in a way that made it not very helpful for some use-cases. I
believe that the change that I made to it makes it strictly more useful
in a compatible way, but let me know if that doesn't seem to be the
case.
While creating this I noticed that our Representable instance for Tuple2
uses true for the first index and false for the second index. I'm not
familiar with any precedents in this area, but I would have expected
false = 0 index and true = 1 index. I assume that this is intended?
I also added several scaladoc examples, because why not?!