`filter` and `filterIndex` names are confusing

[julien-truffaut]

filter and filterIndex are two optic combinators with similar name and behaviour but different types, which may be confusing.

val siblings: Lens[User, List[String]] = ...

siblings.get(user) 
// res: List[String] = List( "Zoe", "Jo", "Eda")

// select list with more than two elements
siblings.filter(_.length > 2).getOption(user)
// res: Option[List[String]] = Some(List( "Zoe", "Jo", "Eda"))

// select names with more than two characters
siblings.each.filter(_.length > 2).getAll(user)
// res: List[String] = List( "Zoe", "Eda")

// select all names except the first (index == 0 means head)
siblings.filterIndex(_ >= 1).getAll(user)
// res: List[String] = List("Jo", "Eda")

It may be easier to see the confusion when we check the type of filter and filterIndex for List

filter     (predicate): Optional [List[A], List[A]]
filterIndex(predicate): Traversal[List[A], A]

[julien-truffaut]

IMO, filter and filterIndex should return the same optic types. For example for List

filter     (predicate: A   => Boolean): Traversal[List[A], A]
filterIndex(predicate: Int => Boolean): Traversal[List[A], A]

One way to get this is to rename the current filter method to select https://github.com/optics-dev/Monocle/blob/master/core/shared/src/main/scala/monocle/Lens.scala#L335-L336 and implement filter in terms of each and select

select(predicate: A => Boolean): Optional[List[A], List[A]]

filter(predicate: A => Boolean): Traversal[List[A], A] = 
  each andThen select(predicate)

[kenbot]

IMHO:

• filter should definitely return Traversal, the intuition for Scala programmers is that it has a Seq => Seq shape
• Seq => (A=> Boolean) => Option[A] is called find in the Scala library
• filterIndex is surely the best name, as long as filter returns Traversal, and so is analogous

[julien-truffaut]

Yeah I completely agree, filter and filterIndex should return a Traversal.

find is a great name but we already have a method find on Fold and Traversal to fetch the first target that matches the predicate https://github.com/optics-dev/Monocle/blob/master/core/shared/src/main/scala/monocle/Traversal.scala#L50-L51

Maybe a real use case would help us to find a good name. I will try to find one tomorrow.

[julien-truffaut]

case class User(name: String, country: String, coins: Int)

val eda = User("Eda", "Germany", 5)
val bob = User("Bob", "UK", 2)

val incrementUKUser =  Optional.foo[User](_.country == "UK").andThen(Focus[User](_.coins)).modify(_ + 10)

incrementUKUser(eda) // do nothing
// res: User = User("Eda", "Germany", 5)

incrementUKUser(bob) 
// res: User = User("Bob", "UK", 12)

I am not sure how should we call foo.

[yilinwei]

Possibly first?

[julien-truffaut]

@yilinwei there is already a first method on some optics, though I am not sure it is very useful

https://github.com/optics-dev/Monocle/blob/master/core/shared/src/main/scala/monocle/Lens.scala#L77

[yilinwei]

I think those look like generic Profunctor method which isn't too useful while you're dealing with a concrete optic.

What about filter1 and filterIndex1, or something closer to the stdlib like findFirst?

[julien-truffaut]

filter(predicate: A   => Boolean): Traversal[List[A], A] =
  each andThen select(predicate)

filter doesn't work well with type inference. Scala cannot figure out the expected type of predicate. We have a similar problem with filterIndex((key: Int) => key > 5), we need to annotate key.

That's why I think we should offer:

def foo(predicate: A   => Boolean): Optional[A, A] =
  Optional[A, A](
    value => if (predicate(value)) Some(value) else None)(
    newValue =>  current => if (predicate(current)) newValue else current
  )

foo is currently called filter which in my opinion is confusing.