Skip to content
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

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Aug 19, 2018

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?!

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?!
@ceedubs
Copy link
Contributor Author

ceedubs commented Aug 19, 2018

@eli-jordan would you be willing to take a look at this and let us know your thoughts?

@codecov-io
Copy link

Codecov Report

Merging #2426 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
core/src/main/scala/cats/Representable.scala 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84e9641...1c7aac8. Read the comment docs.

@eli-jordan
Copy link
Contributor

eli-jordan commented Aug 20, 2018

@ceedubs The change looks good to me. Thanks for adding the doc tests, I probably should have done that originally :-/

Regarding the instance for Tuple2 using true for the left element and false for the right element. This was an arbitrary decision - Representable just mandates there is a "slot" for every inhabitant of the type. I don't have a strong opinion on whether it should be (true, false) of (false, true)

@ceedubs
Copy link
Contributor Author

ceedubs commented Aug 20, 2018

Thanks for the prompt response @eli-jordan. As you said, ultimately the slot assignments for true/false are arbitrary; and I think that changing them at this point would be unnecessarily breaking. Probably best to leave it as it is.

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks! :)

@LukaJCB LukaJCB merged commit b3c704f into typelevel:master Sep 5, 2018
@kailuowang kailuowang added this to the 1.4 milestone Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants