added ifF to Functor

[atais]

Resolving #4144

This is my first contribution, sorry if it's missing anything.

I am also not sure if this is "the correct way" to add this functionality to Functor.
It seemed clean, but on the other hand - I have copied docs for the function, is that ok?

[armanbilge]

👍 Looks good, nice work! :)

[satorg]

I think it may make sense to add a (very simple) test for this change here:
SyntaxSuite.scala

[satorg]

(❗) Before we merge this PR please consider the following:

Perhaps we don't have to do it in this way. This job can be performed by Simulacrum automatically.
Currently Functor#iff is marked as @noop to make Simulacrum skipping this method when generating the syntax.
If we remove that @noop and run Simulacrum against Functor.scala we should get a valid syntax for that method.

Note: the method was marked with @noop in this PR: #3424
Perhaps, it was made intentionally.

[armanbilge]

Perhaps we don't have to do it in this way. This job can be performed by Simulacrum automatically.

Last I heard in #4059 (comment), Simulacrum isn't being run regularly anymore. I certainly don't know how to do it 😆

[satorg]

I certainly don't know how to do it 😆

It should be as simple as

sbt:cats> coreJVM/scalafix TypeClassSupport --files ./core/src/main/scala/cats/Functor.scala

😁

But yeah – it is not run regularly. I think it could be one of the checks for the build – run scalafix against all the files and make sure there're no changes amidst them.

[atais]

(❗) Before we merge this PR please consider the following:

Perhaps we don't have to do it in this way. This job can be performed by Simulacrum automatically. Currently Functor#iff is marked as @noop to make Simulacrum skipping this method when generating the syntax. If we remove that @noop and run Simulacrum against Functor.scala we should get a valid syntax for that method.

Note: the method was marked with @noop in this PR: #3424 Perhaps, it was made intentionally.

it does not work with Simulacrum and the command:

sbt coreJVM/scalafix TypeClassSupport --files ./core/src/main/scala/cats/Functor.scala

it generates in Functor.scala

  trait Ops[F[_], A] extends Serializable {
    ...
    def ifF[B](ifTrue: => B, ifFalse: => B): F[B] = typeClassInstance.ifF[B](self)(ifTrue, ifFalse)
  }

but it does not compile, with an error:

[error] /Users/atais/Documents/cats/core/src/main/scala/cats/Functor.scala:239:78: type mismatch;
[error]  found   : F[A]
[error]  required: F[Boolean]
[error]     def ifF[B](ifTrue: => B, ifFalse: => B): F[B] = typeClassInstance.ifF[B](self)(ifTrue, ifFalse)
[error]                                                                              ^
[error] one error found
[error] one error found

So probably this was the reason behind #3424

[satorg]

but it does not compile, with an error:
So probably this was the reason behind #3424

Ah, indeed: ifF requires F[Boolean] not just F[A]. Sorry, I missed it. But thank you for giving it a try!

[satorg]

Remove : Functor constraint from this method please: it is passed to the ifF method itself, not the IfOps class constructor. Therefore for this method it is completely redundant.

Suggested change

	implicit final def catsSyntaxIfF[F[_]: Functor](fa: F[Boolean]): IfFOps[F] =
	new IfFOps[F](fa)
	implicit final def catsSyntaxIfF[F[_]](fa: F[Boolean]): IfFOps[F] =
	new IfFOps[F](fa)

[armanbilge]

Ah, good catch 😅

[satorg]

I'm thinking... Wouldn't it be better to rename IfFOps and catsSyntaxIfF into something like FunctorBooleanOps and catsSyntaxFunctorBooleanOps? Seems it would closer follow the naming convention used in Cats (see the above FunctorTuple2Ops as an example).

@armanbilge @atais wdyt?

[armanbilge]

I wondered the same thing, but this follows the naming convention used by ifM.

[satorg]

Whatever... I'm ok with both options :)

[satorg]

Well, as @Daenyth suggested in the Discord channel, the difference will affect compiler error messages. Check this out:

import cats.syntax.all._

case class Foo[A](a: A)

Foo(true).ifM(1, 2)

This snippet won't compile since there's no Functor[Foo] defined apparently. But if we leave the implicit method with the Functor constraint:

implicit final def catsSyntaxIfF[F[_]: Functor](fa: F[Boolean]): IfFOps[F] = ???

then the error message will be the following:

Foo(true).ifF(1, 2)
          ^
error: value ifF is not a member of Foo[Boolean]

but if we remove the constraint in the way I suggested initially:

implicit final def catsSyntaxIfF[F[_]](fa: F[Boolean]): IfFOps[F] = ???

then the error will look like

Foo(true).ifF(1, 2)
             ^
error: could not find implicit value for parameter F: cats.Functor[Foo]

Therefore we can justify whether to leave or remove the constraint basing on which error message looks clearer and easier to comprehend:)

[satorg]

@atais @armanbilge what would you prefer?

My personal preference: I would say the latter looks better for me. But I'm not going to insist on it – feel free to merge if you think otherwise.

[atais]

I like the second one more too.

Let me fix that.

Sorry i am not the member of the discord channel and in such situation the link does not open the discussion.

What happened to Gitter, is it not good anymore ;)? At least it was indexed in Google :)

[atais]

squashed my changes and pushed. thanks guys!

[satorg]

Well, there was some discussion some time ago and a decision to step back from Gitter in favor of Discord eventually. I wasn't following all the details but yeah Discord is a better option for now.

[armanbilge]

This became a rather complicated PR for such a simple change, sorry about that! But thanks for it :)

[satorg]

Cannot find a subject for nitpicking anymore )
Thank you, @atais!

[danicheg]

A PR to cats containing new syntax and here is no one mention about mouse within 30 comments?! 🤯 Let's fix it. FWIW, https://github.com/typelevel/mouse is a dedicated project with a bunch of syntaxes. You might want to take a look at it. Tho, I don't mind against this new syntax within cats.