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

Add adaptErr to ApplicativeErrorOps #2698

Merged
merged 3 commits into from
Feb 5, 2019
Merged

Conversation

bplommer
Copy link
Contributor

@bplommer bplommer commented Jan 13, 2019

Partially addresses #2685.

I've changed MonadError#adaptError to be an alias for ApplicativeErrorOps#adaptErr, but I wasn't sure whether I should do this or provide a duplicate implementation.

@codecov-io
Copy link

codecov-io commented Jan 13, 2019

Codecov Report

Merging #2698 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2698      +/-   ##
==========================================
- Coverage   95.16%   95.12%   -0.04%     
==========================================
  Files         365      365              
  Lines        6777     6796      +19     
  Branches      302      293       -9     
==========================================
+ Hits         6449     6465      +16     
- Misses        328      331       +3
Impacted Files Coverage Δ
.../src/main/scala/cats/syntax/applicativeError.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/MonadError.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/syntax/foldable.scala 100% <0%> (ø) ⬆️
...patTest/src/main/scala/catsBC/MimaExceptions.scala 0% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 97.29% <0%> (+0.02%) ⬆️

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 e18523c...7760809. Read the comment docs.

@kailuowang
Copy link
Contributor

thanks so much for taking on this! Here is some feedback

  1. I would duplicate the code with the same comment you have on syntax.
  2. I would leave the laws unchanged and leave that to the future PR that moves the method to the ApplicativeError. The change breaks binary compatibility in cats-laws (which we don't check at the moment), and conventionally everything in XXXXlaws should be in XXXXTests, which we can't do since it's in the syntax ops. Instead, we can add a doctest to the syntax method in ApplicativeErrorOps

@LukaJCB LukaJCB self-requested a review January 26, 2019 14:11
@kailuowang
Copy link
Contributor

Thanks @bplommer

@LukaJCB
Copy link
Member

LukaJCB commented Feb 5, 2019

Looks great, thanks @bplommer! 😊

@LukaJCB LukaJCB merged commit d3b57b2 into typelevel:master Feb 5, 2019
@kailuowang kailuowang added this to the 2.0.0-M1 milestone Apr 18, 2019
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.

4 participants