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 Tuple2K Semigroupal #3502

Merged
merged 3 commits into from
Jul 2, 2020
Merged

Add Tuple2K Semigroupal #3502

merged 3 commits into from
Jul 2, 2020

Conversation

rmehri01
Copy link
Contributor

Closes #3410.

I wasn't really sure where in the Tuple2kInstances this should be added so I just put it in the last one so please tell me if there is somewhere else it should go. Also, should tests be added because I had a bit of trouble doing so. Thanks!

LukaJCB
LukaJCB previously approved these changes Jun 29, 2020
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Actually before we merge this, can we make Tuple2KSemigroupal a superclass for Tuple2KApply?

@rmehri01
Copy link
Contributor Author

rmehri01 commented Jun 29, 2020

Alright I can do that. Just another thing, does it matter which tuple2k instance abstract class this goes in because there was 0-8 so I wasn't really sure and also should this be tested?

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2020

Codecov Report

Merging #3502 into master will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3502      +/-   ##
==========================================
- Coverage   91.77%   91.76%   -0.02%     
==========================================
  Files         383      383              
  Lines        8400     8401       +1     
  Branches      218      216       -2     
==========================================
  Hits         7709     7709              
- Misses        691      692       +1     

@rmehri01
Copy link
Contributor Author

Also is that import at the top of the file import cats.Contravariant needed? Intellij says it isn't used.

@joroKr21
Copy link
Member

Alright I can do that. Just another thing, does it matter which tuple2k instance abstract class this goes in because there was 0-8 so I wasn't really sure and also should this be tested?

This is the correct place. I'm not sure about testing, I guess you would need a type that has Semigroupal but not Applicative to test this.

@rmehri01
Copy link
Contributor Author

rmehri01 commented Jun 29, 2020

Alright I can do that. Just another thing, does it matter which tuple2k instance abstract class this goes in because there was 0-8 so I wasn't really sure and also should this be tested?

This is the correct place. I'm not sure about testing, I guess you would need a type that has Semigroupal but not Applicative to test this.

Ah okay thanks for the response, yeah that makes sense. Also does it matter which tuple2k instances class this goes in because I'm not really sure why they are labelled 0-8, I thought maybe it was because they had some sort of structure.

@joroKr21
Copy link
Member

Also does it matter which tuple2k instances class this goes in because I'm not really sure why they are labelled 0-8, I thought maybe it was because they had some sort of structure.

It matters but this is the correct place to put it. The reason they exist is implicit priority. Semigroupal should have the lowest priority (same as Functor) because it has the least constraints. There is also no overlap between Functor and Semigroupal so they can be in one place without clashing.

@rmehri01
Copy link
Contributor Author

It matters but this is the correct place to put it. The reason they exist is implicit priority. Semigroupal should have the lowest priority (same as Functor) because it has the least constraints. There is also no overlap between Functor and Semigroupal so they can be in one place without clashing.

Ah gotcha, thank you! :)

@rmehri01 rmehri01 requested a review from LukaJCB June 30, 2020 04:23
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@LukaJCB LukaJCB merged commit 807b979 into typelevel:master Jul 2, 2020
@rmehri01
Copy link
Contributor Author

rmehri01 commented Jul 2, 2020

Np, thanks for the help everyone! :)

@rmehri01 rmehri01 deleted the tuple2k-semigroupal branch July 2, 2020 03:05
@travisbrown travisbrown added this to the 2.2.0-RC1 milestone Jul 3, 2020
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.

Missing TupleK Semigroupal
7 participants