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 tests to cover derived methods leftMap and rightMap on BiFunctor #652

Merged
merged 2 commits into from
Nov 15, 2015

Conversation

mikejcurry
Copy link
Contributor

This PR would add tests that should cover identity for the derived leftMap and rightMap on BiFunctor.

I guess this PR is exploratory:

  • Is this a suitable use of the laws infrastructure? Seems to be a good use, but not sure if you would actually consider them laws.

Also:

  • There is no syntax for leftMap and rightMap at this stage, just bimap; would it make sense to add these as syntax also. I’ve not done that in this PR, but if it is generally in line with what we want to do I can add.

Add tests to cover derived methods leftMap and rightMap
@mikejcurry mikejcurry changed the title Add tests to cover derived methods leftMap and rightMap Add tests to cover derived methods leftMap and rightMap on BiFunctor Nov 15, 2015
@ceedubs
Copy link
Contributor

ceedubs commented Nov 15, 2015

👍 thanks!

Is this a suitable use of the laws infrastructure?

At some point #370 may provide some answers to this. However, until we have something better in place, I think these sorts of checks are good. There is also a precedent here.

There is no syntax for leftMap and rightMap at this stage, just bimap; would it make sense to add these as syntax also.

Yes, I think so. We would inherit them for free if Simulacrum supported type classes for type constructors of kind F[_, _]. Since it doesn't, it would probably be handy to add them manually.

@codecov-io
Copy link

Current coverage is 78.27%

Merging #652 into master will increase coverage by +1.08% as of a16bd2d

@@            master    #652   diff @@
======================================
  Files          159     159       
  Stmts         2083    2108    +25
  Branches        70      70       
  Methods          0       0       
======================================
+ Hit           1608    1650    +42
  Partial          0       0       
+ Missed         475     458    -17

Review entire Coverage Diff as of a16bd2d

Powered by Codecov. Updated on successful CI builds.

@mikejcurry
Copy link
Contributor Author

@ceedubs - Cool, Thanks. I've gone ahead and locally made changes to add leftMap and rightMap to syntax, added associativity checks for leftMap and rightMap, and updated the tests to use the syntax additions. Would you folks prefer pushing to this PR or a separate PR after this one?

@ceedubs
Copy link
Contributor

ceedubs commented Nov 15, 2015

@mikejcurry I have no real preference. I have no objections to you adding those changes to this PR. The more changes you add to a PR the more likely one of them will raise objections, but these seem pretty uncontroversial to me :)

@mikejcurry
Copy link
Contributor Author

@ceedubs Sounds good - I'll push the changes to this PR and keep the set of changes in the same area together.

@ceedubs
Copy link
Contributor

ceedubs commented Nov 15, 2015

👍

1 similar comment
@non
Copy link
Contributor

non commented Nov 15, 2015

👍

non added a commit that referenced this pull request Nov 15, 2015
Add tests to cover derived methods leftMap and rightMap on BiFunctor
@non non merged commit 595e8d8 into typelevel:master Nov 15, 2015
@mikejcurry mikejcurry deleted the bifunctor-laws branch December 11, 2015 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants