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.

Sign up for GitHub

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

Jump to bottom

Add InjectK laws #1725

Merged
merged 2 commits into from
Jun 12, 2017
Merged

Add InjectK laws #1725

merged 2 commits into from
Jun 12, 2017

Conversation

andyscott
Copy link
Contributor

This addresses issue #1645.

@andyscott andyscott mentioned this pull request Jun 9, 2017
Merged
3 tasks
kailuowang
kailuowang reviewed Jun 9, 2017
View reviewed changes
laws/src/main/scala/cats/laws/InjectKLaws.scala Outdated
(injectK.prj compose injectK.inj).apply(fa) <-> Some(fa)

def injectKRoundTripPrj[A](ga: G[A]): IsEq[Option[G[A]]] =
injectK.prj(ga).map(fa => injectK.inj(fa)) <-> Some(ga)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's lawful for prj to return a None, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! This commit is goofy-- I cherry picked it from ages ago and I'm reworking it now.

That being said, how do you check this kind of behavior with discipline? Would it suffice to just have the injectKRoundTripInj and nix injectKRoundTripPrj?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say when it returns a Some you should check the roundtrip right?

@codecov-io
Copy link

codecov-io commented Jun 9, 2017 •
edited
Loading

Codecov Report

Merging #1725 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1725      +/-   ##
=========================================
+ Coverage   93.88%   93.9%   +0.01%     
=========================================
  Files         246     248       +2     
  Lines        4088    4099      +11     
  Branches      154     156       +2     
=========================================
+ Hits         3838    3849      +11     
  Misses        250     250
Impacted Files Coverage Δ
laws/src/main/scala/cats/laws/InjectKLaws.scala 100% <100%> (ø)
...main/scala/cats/laws/discipline/InjectKTests.scala 100% <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 1931c93...13ac20c. Read the comment docs.

kailuowang
kailuowang approved these changes Jun 12, 2017
View reviewed changes
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 very much!

@kailuowang kailuowang merged commit 3944108 into typelevel:master Jun 12, 2017
@andyscott andyscott deleted the feature/injectk-laws branch June 12, 2017 19:30
@kailuowang kailuowang modified the milestone: 1.0.0-MF Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djspiewak djspiewak djspiewak approved these changes

@kailuowang kailuowang kailuowang approved these changes

@edmundnoble edmundnoble edmundnoble approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0-MF
Development

Successfully merging this pull request may close these issues.
5 participants
@andyscott @codecov-io @djspiewak @kailuowang @edmundnoble