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

Rename prod coproduct #1589

Merged
merged 2 commits into from
Apr 5, 2017
Merged

Conversation

kailuowang
Copy link
Contributor

fixes #1582
Note that this should be a fairly straightforward PR to review. However, github failed to render the diff in renamed files correctly - it displays the diff as a whole file removed and a new file. To overcome this, you might want to review the two commits separately.

@johnynek
Copy link
Contributor

johnynek commented Apr 5, 2017

thanks for doing this! I think this is a win for consistency and readability prior to 1.0.

👍

@codecov-io
Copy link

codecov-io commented Apr 5, 2017

Codecov Report

Merging #1589 into master will not change coverage.
The diff coverage is 98.87%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1589   +/-   ##
=======================================
  Coverage   92.42%   92.42%           
=======================================
  Files         249      249           
  Lines        3974     3974           
  Branches      131      131           
=======================================
  Hits         3673     3673           
  Misses        301      301
Impacted Files Coverage Δ
free/src/main/scala/cats/free/Free.scala 88.23% <ø> (ø) ⬆️
core/src/main/scala/cats/data/Nested.scala 96.15% <ø> (ø) ⬆️
core/src/main/scala/cats/arrow/FunctionK.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/Inject.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/EitherK.scala 100% <100%> (ø)
core/src/main/scala/cats/data/Func.scala 100% <100%> (ø) ⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 89.83% <100%> (ø) ⬆️
core/src/main/scala/cats/syntax/eitherK.scala 100% <100%> (ø)
core/src/main/scala/cats/data/Tuple2K.scala 96.66% <96.66%> (ø)
... and 2 more

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 dbe4407...7c1329e. Read the comment docs.

Copy link
Contributor

@edmundnoble edmundnoble left a comment

Choose a reason for hiding this comment

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

LGTM.

@adelbertc
Copy link
Contributor

Bikeshedding, but Tuple2K feels weird even though it's correct since it's a higher-order Tuple2, but what about just TupleK ?

@kailuowang
Copy link
Contributor Author

kailuowang commented Apr 5, 2017

@adelbertc I don't dislike TupleK. I picked Tuple2K exactly due to the correctness, and TupleK sounds like it's arity free?

@adelbertc
Copy link
Contributor

yeahhhhh i just don't like having numbers in names for some reason.. but oh well 👍

@kailuowang kailuowang merged commit 2a32e37 into typelevel:master Apr 5, 2017
@edmundnoble
Copy link
Contributor

@adelbertc I thought PairK might be nice for that reason, but that might drive people to use scala.Predef.Pair.

@kailuowang kailuowang deleted the rename-prod-coproduct branch April 6, 2017 12:32
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
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.

rename Prod / Coproduct to ProductK / CoproductK
5 participants