Refactor toMap

[zsxwing]

A PR to refactor toMap.

There is a breaking change in this PR:

• Remove def toMap[K, V] (keySelector: T => K, valueSelector: T => V, mapFactory: () => Map[K, V]): Observable[Map[K, V]]

The other changes are still source compatible and binary compatible.

[samuelgruetter]

I like the idea, but please give me some time to check/think about the details ;-)

Moreover, I'd like to know if anything of this PR is a breaking change. It would be nice to have a comparison of what {works/doesn't work} {with/without} this PR. Could then also be copied to the release notes of the first version containg these changes.

[samuelgruetter]

scala.collection.immutable.TreeMap ---> immutable.TreeMap

[zsxwing]

I updated the description and override the PR with a formal commit. Sorry that I should have done it at first.

[zsxwing]

having a default value for an implicit seems a bit dangerous at first sight.

You're right. Thanks for pointing out it. I created a helper method

def to[K, V, M[_, _]](keySelector: T => K, valueSelector: T => V)(implicit cbf: CanBuildFrom[Nothing, (K, V), M[K, V]]): Observable[M[K, V]] = {

and used to implement toMap. This will make toXXXMap be consistent with toList, toSeq.

[samuelgruetter]

Btw did you try out if you can implement toMap just as

this.map(elem => (keySelector(elem), valueSelector(elem))).to[Map]

[zsxwing]

It won't work. Here is the signature of to:

def to[Col[_]](implicit cbf: CanBuildFrom[Nothing, T, Col[T @uncheckedVariance]]): Observable[Col[T @uncheckedVariance]]

It expects a Col with one type parameter and will return Col[T].
this.map(elem => (keySelector(elem), valueSelector(elem))).to[Map] won't work because:

• Map has two type parameters
• toMap should return Observable[Map[K, V]] instead of Observable[Map[(K, V)]]

[samuelgruetter]

I see.
Something else: Let's add the toMap of Scala Lists:

def toMap[T, U](implicit ev: A <:< (T, U)): Map[T, U] 

[samuelgruetter]

If you remove this example, you should show how we can do this now. Not necessarily with a non-empty starting map, but maybe with a HashMap from the collection library?

[zsxwing]

Let's add the toMap of Scala Lists:

Done

No need to clutter the docs with immutable. prefixes

Done

If you remove this example, you should show how we can do this now.

Added two examples to show how to use to[K, V, M[_, _]]. However, I cannot find a way to make the compiler infer the K, V types.

I'm not sure if we should have these toXxxMap methods.

Maybe it's better not to add toXxxMap methods in this PR?

[samuelgruetter]

Maybe it's better not to add toXxxMap methods in this PR?

Yes, I think so.

[samuelgruetter]

I don't quite like these two examples because none of them shows explicitly what's happening. I'd only make one example, but add all types there:

    val m: Observable[HashMap[Char, String]] = o.to[Char, String, HashMap](keySelector, valueSelector)

IMHO that's more readable.

[samuelgruetter]

And while I'm reading this, I wonder why we don't swap the order of the type params:

o.to[HashMap, Char, String](keySelector, valueSelector)

corresponds better to the order of pronouncing what we're doing: we say "convert o to a HashMap of Char/String".

[samuelgruetter]

We should not forget to make toMultimap consistent with toMap, but we can also do that in a separate PR.

[zsxwing]

swap the order of the type params

Sounds great. Done.

[zsxwing]

I cannot figure out how to refactor toMultimap now. Here is my attempt. However, the problem is how to add value to b?

  def to[M[_, _], Col[_], K, V](keySelector: T => K, valueSelector: T => V)
                               (implicit mcbf: CanBuildFrom[Nothing, (K, Col[V]), M[K, Col[V]]], 
                                cbf: CanBuildFrom[Nothing, V, Col[V]]): Observable[M[K, Col[V]]] = {
    lift {
      (subscriber: Subscriber[M[K, Col[V]]]) => {
        new Subscriber[T](subscriber) {
          val b = mcbf()

          override def onStart(): Unit = request(Long.MaxValue)

          override def onNext(t: T): Unit = {
            val key = keySelector(t)
            val value = valueSelector(t)
            // how to add value to b?
            // b expects (K, Col[V])
          }

          override def onError(e: Throwable): Unit = {
            subscriber.onError(e)
          }

          override def onCompleted(): Unit = {
            subscriber.onNext(b.result)
            subscriber.onCompleted()
          }
        }
      }
    }
  }

[samuelgruetter]

We could also use scala.collection.mutable.MultiMap, which has an addBinding method. Since it's mutable, there's no need to use a builder, we could just start with an empty mutable multimap and add one key/value pair after the other using addBinding. The question is, however, to what extent we can/want to abstract over the Map type and Set type used by MultiMap.

[zsxwing]

Sent another PR #68 for toMultimap as per your suggestion.

[samuelgruetter]

Did you try if you can define a default argument of v => v for valueSelector? Not sure if this can work because scalac would have to infer from this that T must be the same as V.

[zsxwing]

not work...

[zsxwing]

@samuelgruetter further suggestion?

[samuelgruetter]

LGTM