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 init and size methods to NonEmptyList #1628

Merged
merged 4 commits into from
Apr 26, 2017

Conversation

jtjeferreira
Copy link
Contributor

No description provided.

* }}}
*/
def init: List[A] = tail match {
case Nil => List.empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be head :: Nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the tail is empty the last element is the head, so the result is the empty list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterneyens I added tests for init and size

@codecov-io
Copy link

codecov-io commented Apr 24, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1628      +/-   ##
==========================================
+ Coverage   93.37%   93.37%   +<.01%     
==========================================
  Files         240      240              
  Lines        3937     3941       +4     
  Branches      139      144       +5     
==========================================
+ Hits         3676     3680       +4     
  Misses        261      261
Impacted Files Coverage Δ
core/src/main/scala/cats/data/NonEmptyList.scala 97.22% <100%> (+0.1%) ⬆️

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 97a3d26...63f74f1. Read the comment docs.

* scala> nel.last
* res0: Int = 5
* }}}
*/
def last: A = tail.lastOption match {
case None => head
case Some(a) => a
Copy link
Contributor

Choose a reason for hiding this comment

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

tail.lastOption.getOrElse(head) might be more performant.

Copy link
Contributor

Choose a reason for hiding this comment

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

he got the exact opposite advice from me a few days ago. Why do you say this? My understanding is that case class matching is very efficient and this also avoids the closure allocation (since getOrElse is by-name).

Copy link
Contributor

@kailuowang kailuowang Apr 26, 2017

Choose a reason for hiding this comment

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

@johnynek yeah, I forgot that getOrElse is by-name, and corrected myself in the other comment #1628 (comment)

* res0: scala.collection.immutable.List[Int] = List(1, 2, 3, 4)
* }}}
*/
def init: List[A] = tail match {
Copy link
Contributor Author

@jtjeferreira jtjeferreira Apr 24, 2017

Choose a reason for hiding this comment

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

@kailuowang any preference regarding init performance? toList.init is also an alternative

Copy link
Contributor

Choose a reason for hiding this comment

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

I am going to stop pretend that I know anything about performance. I don't know if much difference can be made here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It (should) be more performant this way I think, because there's one less branch. But only very slightly.

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.

Nice change!

* }}}
*/
def last: A = tail.lastOption.getOrElse(head)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't change this implementation. The one above avoids the allocation of the closure and was the motivation for writing it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will revert

Copy link
Contributor

Choose a reason for hiding this comment

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

oops, I forgot that getOrElse is by-name. 👅 we need a strict getOrElese.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 25, 2017

👍 from me after https://github.com/typelevel/cats/pull/1628/files#r112997682 is addressed.

@johnynek
Copy link
Contributor

👍

@johnynek johnynek merged commit 58d680f into typelevel:master Apr 26, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-MF May 18, 2017
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.

7 participants