Add size to Foldable

[afiore]

Partially addresses what discussed in #1091.

[afiore]

Obviously, this naive implementation would not terminate on infinite streams. @yilinwei was suggesting that a possible approach could be to have size return an Option[Long], which would allow tweaking the implementation for specific instances (e.g. Streams could return None).

[non]

@afiore I would be fine with that behavior. On the other hand, we already have potentially non-terminating methods (like .toList) so I am fine with this method being unsafe as well (as long as we note that in the documentation).

[ceedubs]

I like this approach as opposed to the Option approach. I think that if we want to have support for potentially infinite structures, it should be via a separate method that takes a bound so you can say something like "give me the size, but cap it at 1,000 instead blocking forever on an infinite stream" or something. I think we should keep that out of the scope of this PR though.

I think that this is a great start. There are some additional things that I think should be done:

• Add a ScalaDoc comment explaining that the default implementation is a naive fold over the structure meaning that 1) it may not terminate and 2) instances for structures that have efficient size implementations should override this
• Override size in instances for types that have efficient size implementations (Vector comes to mind - are there others?).

Would you be willing to do those? If not, I'm sure that someone else can get them in a followup PR.

[ceedubs]

👍 I think that Long was a good choice here. Int is probably the first thing that would have come to my mind, but people could conceivably be using Foldable for some very large structures. Though hopefully if their structure is large enough to pass the max Int value they have overridden size to a more efficient implementation :)

[ceedubs]

A couple other structures came to mind that could benefit from an override of size: Set, Map, andOneAnd. There may be marginal performance improvements forListandStream`, but it probably doesn't matter much for those.

[codecov-io]

Current coverage is 88.70%

Merging #1114 into master will increase coverage by 0.05%

1. 2 files (not in diff) in ...main/scala/cats/data were modified. more
2. 5 files (not in diff) in .../src/main/scala/cats were modified. more
   • Misses +3
   • Hits +7
3. 2 files in .../src/main/scala/cats were modified. more
   • Misses -4
   • Hits -6

@@             master      #1114   diff @@
==========================================
  Files           227        227          
  Lines          2953       2958     +5   
  Methods        2897       2898     +1   
  Messages          0          0          
  Branches         53         57     +4   
==========================================
+ Hits           2618       2624     +6   
+ Misses          335        334     -1   
  Partials          0          0          

Powered by Codecov. Last updated by b149835...15a0c4d

[afiore]

@ceedubs I have added overrides for Map, Set and Vector using the standard library implementation of size, which I believe is defined in scala.collection.TraversableOnce. Not sure I understand how providing an override for OneAnd would help. Wouldn't it just use the instance corresponding to the structure used for the tail (e.g. Foldable[Vector] for OneAnd[Vector, A])?

[ceedubs]

Not sure I understand how providing an override for OneAnd would help. Wouldn't it just use the instance corresponding to the structure used for the tail (e.g. Foldable[Vector] for OneAnd[Vector, A])?

@afiore It will use a foldMap derived from the first element plus the foldLeft on the tail structure. If instead there were an override for 1 + size(tail), then it could take advantage of any size optimizations that may be in the Foldable[F] type class for the tail (such as with Vector). Does that make sense?

[afiore]

@ceedubs makes sense! I have amended my commit adding an override for AndOne.

[ceedubs]

This seems to be duplicating the implementation within the test. Instead, it might be better to do something like oa.size should === (oa.unwrap.size) or oa.size should === (oa.toList.size).

[afiore]

@ceedubs I have updated AndOneTests with an assertion that doesn't repeat the actual implementation.

[ceedubs]

👍 thanks, @afiore!

[non]

👍