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 NonEmptyMap#toNonEmptyList (issue #2346) #2535

Merged
merged 1 commit into from
Sep 27, 2018

Conversation

CucumisSativus
Copy link
Contributor

Hello, this is my first pull request there so please go easy with me :)

As for #2346 started with small steps - implementing just first method. Frankly i have no idea if I placed this in proper place, no idea if my test is sufficient and no idea if the thing I implemented is performant and if my git message is sufficient. Would love any feedback from you. Then after initial feedback I will do my best to implement rest of the methods from that issue


test("NonEmptyMap#toNonEmptyList is consistent with Map#toList and creating NonEmptyList from it"){
forAll{ nem: NonEmptyMap[String, Int] =>
nem.toNonEmptyList should ===(NonEmptyList.fromListUnsafe(nem.toSortedMap.toList))
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately the toNonEmptyList of NonEmptyMap collides with toNonEmptyList of Reducible, so the nem.toNonEmptyList here seems to return a NonEmptyList[Int] instead of the NonEmptyList[(String, Int)] that we want.
Maybe we should rename the method to toNel instead? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worked, thanks a lot!

Copy link
Contributor

@easel easel left a comment

Choose a reason for hiding this comment

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

LGTM, welcome to cats!

@codecov-io
Copy link

Codecov Report

Merging #2535 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
+ Coverage   95.18%   95.19%   +<.01%     
==========================================
  Files         360      360              
  Lines        6548     6549       +1     
  Branches      279      275       -4     
==========================================
+ Hits         6233     6234       +1     
  Misses        315      315
Impacted Files Coverage Δ
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 96% <100%> (+0.05%) ⬆️

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 cc8919f...66d8957. Read the comment docs.

@kailuowang kailuowang merged commit 4acdbb9 into typelevel:master Sep 27, 2018
@kailuowang kailuowang added this to the 1.5 milestone Oct 30, 2018
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