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 groupBy for NonEmptySet #2285

Merged
merged 1 commit into from
Jun 14, 2018
Merged

Conversation

Obarros
Copy link
Contributor

@Obarros Obarros commented Jun 9, 2018

Should fixe #2282.

@LukaJCB PTAL.

* Groups elements inside this `NonEmptySet` according to the `Order`
* of the keys produced by the given mapping function.
*/
def groupBy[B](f: A => B)(implicit B: Order[B]): NonEmptyMap[B, NonEmptySet[A]] = {
Copy link
Member

Choose a reason for hiding this comment

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

If we're returning NonEmptyMap here, does it make sense to make NonEmptyList#groupBy also return a non-empty map? @LukaJCB

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's probably true, but we can't change it without breaking compatibility

@kubukoz
Copy link
Member

kubukoz commented Jun 9, 2018 via email

@codecov-io
Copy link

codecov-io commented Jun 9, 2018

Codecov Report

Merging #2285 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2285      +/-   ##
==========================================
+ Coverage   95.03%   95.05%   +0.02%     
==========================================
  Files         337      338       +1     
  Lines        5840     5850      +10     
  Branches      222      220       -2     
==========================================
+ Hits         5550     5561      +11     
+ Misses        290      289       -1
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/set.scala 100% <100%> (ø)
core/src/main/scala/cats/data/NonEmptySet.scala 98.75% <100%> (+1.45%) ⬆️

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 c712a65...208b238. Read the comment docs.

LukaJCB
LukaJCB previously approved these changes Jun 9, 2018
@kailuowang
Copy link
Contributor

to make this available with import cats.implicits._ , we also need to add this to this
binary compatible trait

@LukaJCB
Copy link
Member

LukaJCB commented Jun 11, 2018

@kailuowang
Copy link
Contributor

Sorry I missed that.

kailuowang
kailuowang previously approved these changes Jun 11, 2018
@LukaJCB
Copy link
Member

LukaJCB commented Jun 12, 2018

What we did forget is to add a specific import for these syntax extensions, we should add an object set extends SetSyntax to here: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/syntax/package.scala#L46

@Obarros

@Obarros
Copy link
Contributor Author

Obarros commented Jun 12, 2018

@LukaJCB any test that we can have for this?

@Obarros Obarros dismissed stale reviews from kailuowang and LukaJCB via 792b736 June 12, 2018 17:58
@kubukoz
Copy link
Member

kubukoz commented Jun 12, 2018

@Obarros I think there is a spec that checks if syntax compiles. Look for SyntaxSuite :)

I think the list syntax isn't there, so we could add that now that we're at it

@Obarros Obarros force-pushed the groupByNES-2282 branch 2 times, most recently from 2ac680e to 7ce57d8 Compare June 13, 2018 16:45
@Obarros
Copy link
Contributor Author

Obarros commented Jun 13, 2018

Ping @kubukoz @LukaJCB!
Updated the SyntaxSuite with list and set syntax.

@kubukoz
Copy link
Member

kubukoz commented Jun 13, 2018

looks good, :shipit:

val grouped: SortedMap[B, NonEmptySet[A]] = set.groupByNes(f)
}

def testNonEmptyList[A: Order, B: Order] : Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need this Order constraint on A, leaving it on B should be enough. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @LukaJCB.
Let me remove it, I will also rebase on top of the latest master before squashing the commits and push.

@kailuowang
Copy link
Contributor

@Obarros, it's no big deal but it's not necessary to squash before merge. We usually do squash merge (meaning all commits on pr will be squashed on merge by GitHub) . Having separate commits helps reviewing the latest changes.

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.

Thanks again @Obarros! Merging.

@LukaJCB LukaJCB merged commit daa646d into typelevel:master Jun 14, 2018
@kubukoz kubukoz mentioned this pull request Jun 14, 2018
@kailuowang kailuowang added this to the 1.2 milestone Jun 14, 2018
@Obarros Obarros deleted the groupByNES-2282 branch June 16, 2018 13:04
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.

5 participants