Skip to content

Introduce Bifunctor Laws#559

Merged
fthomas merged 2 commits into
typelevel:masterfrom
agenovese:Issue557
Oct 4, 2015
Merged

Introduce Bifunctor Laws#559
fthomas merged 2 commits into
typelevel:masterfrom
agenovese:Issue557

Conversation

@agenovese
Copy link
Copy Markdown

@agenovese agenovese commented Oct 4, 2015

  • Create BifuncorLaws
  • Create BifunctorTests
  • Add Bifunctor instances for Validated, Ior, Xor, and XorT
  • Add Bifunctor tests for Validated, Ior, Xor, and XorT

Fixes: 557

@angelo-streetcontxt
Introduce Bifunctor Laws
1af9ec4 
- Create BifuncorLaws
- Create BifunctorTests
- Add Bifunctor instances for Validated, Ior, Xor, and XorT
- Add Bifunctor tests for Validated, Ior, Xor, and XorT

Fixes: 557
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 4, 2015

Current coverage is 75.89%

Merging #559 into master will increase coverage by +0.41% as of 56da30a

@@            master    #559   diff @@
======================================
  Files          153     156     +3
  Stmts         2064    2112    +48
  Branches        68      68       
  Methods          0       0       
======================================
+ Hit           1558    1603    +45
  Partial          0       0       
- Missed         506     509     +3

Review entire Coverage Diff as of 56da30a

Powered by Codecov. Updated on successful CI builds.

@non
Copy link
Copy Markdown
Contributor

non commented Oct 4, 2015

Thanks! This looks good to me.

👍

ceedubs
ceedubs reviewed Oct 4, 2015
View reviewed changes
Comment thread core/src/main/scala/cats/data/Ior.scala Outdated
Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Oct 4, 2015

Choose a reason for hiding this comment

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

Sorry, this is super nitpicky, but could you name this iorBifunctor just to match the convention of the instances above it?

Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Oct 4, 2015

Choose a reason for hiding this comment

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

Similarly for the instances in the other files.

@ceedubs
Copy link
Copy Markdown
Contributor

ceedubs commented Oct 4, 2015

Other than my super nitpicky comment, this looks good. 👍

fthomas
fthomas reviewed Oct 4, 2015
View reviewed changes
Comment thread core/src/main/scala/cats/data/Ior.scala Outdated
Copy link
Copy Markdown
Member

@fthomas fthomas Oct 4, 2015

Choose a reason for hiding this comment

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

Could you also remove the parens from f and g? If functions have only one parameter we usually omit them.

@fthomas
Copy link
Copy Markdown
Member

fthomas commented Oct 4, 2015

Also LGTM.

@angelo-streetcontxt
Address Code Review feedback
786d532 
- Removed unnecessary  parens
- Renamed methods to match the convention
@agenovese
Copy link
Copy Markdown
Author

agenovese commented Oct 4, 2015

Fixed, and sorry about those. I should have noticed the naming and I never use parens like that (going to blame the IDE and the fact that it was after 2am).

@fthomas
Copy link
Copy Markdown
Member

fthomas commented Oct 4, 2015

Merging, thanks!

fthomas added a commit that referenced this pull request Oct 4, 2015
@fthomas
Merge pull request #559 from agenovese/Issue557
3565ed4 
Introduce Bifunctor Laws
@fthomas fthomas merged commit 3565ed4 into typelevel:master Oct 4, 2015
@fthomas fthomas mentioned this pull request Oct 4, 2015
Closed
@agenovese agenovese deleted the Issue557 branch October 4, 2015 15:52
@ceedubs
Copy link
Copy Markdown
Contributor

ceedubs commented Oct 5, 2015

@agenovese no worries. What you had was completely fine! I'm just nitpicky :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@agenovese @codecov-io @non @ceedubs @fthomas @angelo-streetcontxt