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 count as syntax to UnorderedFoldable #2520

Merged
merged 4 commits into from
Sep 25, 2018
Merged

Add count as syntax to UnorderedFoldable #2520

merged 4 commits into from
Sep 25, 2018

Conversation

leusgalvan
Copy link
Contributor

Fixes #2447 .

@codecov-io
Copy link

codecov-io commented Sep 22, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2520      +/-   ##
==========================================
+ Coverage   95.35%   95.36%   +<.01%     
==========================================
  Files         358      359       +1     
  Lines        6530     6532       +2     
  Branches      282      280       -2     
==========================================
+ Hits         6227     6229       +2     
  Misses        303      303
Impacted Files Coverage Δ
testkit/src/main/scala/cats/tests/CatsSuite.scala 70% <ø> (ø) ⬆️
...src/main/scala/cats/syntax/unorderedFoldable.scala 100% <100%> (ø)

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 1d5a51a...f0ef069. Read the comment docs.

@LukaJCB
Copy link
Member

LukaJCB commented Sep 22, 2018

Hi @leusgalvan thanks a lot for contributing! Is there a reason you're using the underscore syntax, why not just count? :)

@leusgalvan
Copy link
Contributor Author

@LukaJCB only because when I tried to use it in a simple way, the count method from the standard library (which returns an Int) gets called instead of the cats one (which returns Long):

import cats._
import cats.implicits._
Set("hi", "world").count(_.length > 3)
res0: Int = 1

* res2: Long = 1
* }}}
*/
def count_(p: A => Boolean)(implicit F: UnorderedFoldable[F]): Long =
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it'd be better to have count defined in the UnorderedFoldable trait itself, and only call count here? Otherwise you can only use it with the syntax implicits, and not just F.count

Copy link
Member

Choose a reason for hiding this comment

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

You can't do that if you want to maintain binary compat.

Copy link
Member

Choose a reason for hiding this comment

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

Aww damn :( the encoding of traits strikes again

Copy link
Member

Choose a reason for hiding this comment

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

we should be able to define extensions for typeclass instances, like:

implicit class UFOps[F[_]](val instance: UnorderedFoldable[F]) extends AnyVal {
  def count(...)
}

@LukaJCB
Copy link
Member

LukaJCB commented Sep 22, 2018

@leusgalvan I think I'd prefer to just call it count, we have a convention where an underscore after a name usually means it's some kind of operation that doesn't quite do what the same function without the underscore would do.

See filter_, takeWhile_, traverse_, etc.

@kubukoz
Copy link
Member

kubukoz commented Sep 22, 2018

@LukaJCB if it's called count, using it with a concrete type that has count defined on it will make it impossible to use - similar to fold and combineAll.

Although it may be alright to keep it that way here, because there's a perfectly sane count on Lists and similar types that works the same as the one here...

@leusgalvan
Copy link
Contributor Author

@kubukoz @LukaJCB Ok let's go with count.

@LukaJCB LukaJCB changed the title Add count_ as syntax to UnorderedFoldable Add count as syntax to UnorderedFoldable Sep 23, 2018
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.

Looks great, one small thing is that I think we should add this to the cats.syntax package object :)

@LukaJCB LukaJCB merged commit 4178665 into typelevel:master Sep 25, 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.

6 participants