kotlinx.collections.immutable icon indicating copy to clipboard operation
kotlinx.collections.immutable copied to clipboard

Fixed `toPersistentSet`, `toPersistentMap` to work with any impl.

Open comahe-de opened this issue 3 years ago • 2 comments

toPersistentSet and toPersistentMap now working in a similar way as toPersistentList

The contract in the KDoc was not fulfilled as it says for toPersistentSet:

If the receiver is already a persistent set, returns it as is. If the receiver is a persistent set builder, calls build on it and returns the result.

But the previous implementation just checked for PersistentOrderedSet, other implementations of PersistentSet where not considered

Similar problem war fixed for toPersistentMap

comahe-de avatar May 08 '21 12:05 comahe-de

toPersistentSet/Map intensionally checks only for the ordered set. The idea here is to be consistent with stdlib toMutableSet(), which always returns an ordered set.

Can you please share your use case and why working with any implementation is a better behavior?

qurbonzoda avatar May 14 '21 02:05 qurbonzoda

@qurbonzoda I have some custom implementations of PersistentList/-Set/-Map that mainly delegate to the default PersistentList/-Set/-Map but add some additional properties. While a call to toPersistentList() on my PersistentList implementation returns the same object, a call to toPersistentSet() on my PersistentSet implementation returns a new object, which was really unexpected as the doc says If the receiver is already a persistent set, returns it as is.

Regarding toMutableSet(): this method has a complete other behaviour: it always returns a new Set even if is already a Set. So the name is not well chosen. toNewMutableSet() would be a better name. But this is a another topic.

comahe-de avatar May 15 '21 20:05 comahe-de