Discussion regarding Fluent.collect return type about flupy HOT 4 CLOSED

awesome! thanks for the research & examples

We could instead support some standard library types with the overloads operator, such as Tuple, List, Set.

I agree, that would easily handle the 99% use-case

But (afaik) it runs up against another limitation. Here's a smaller repro case:

from typing import Callable, Collection, Iterable, List, Set, TypeVar, overload

T = TypeVar("T")

CollectionTakesIterT = Callable[[Iterable[T]], Collection[T]]
ListTakesIterT = Callable[[Iterable[T]], List[T]]
SetTakesIterT = Callable[[Iterable[T]], Set[T]]


@overload
def collect(xs: Iterable[T], container_type=ListTakesIterT) -> List[T]:
    ...


@overload
def collect(xs: Iterable[T], container_type=SetTakesIterT) -> Set[T]:
    ...


def collect(xs: Iterable[T], container_type=CollectionTakesIterT) -> Collection[T]:
    return container_type(xs)

Result:

Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same or broader

I was hoping to resolve that by passing Literal[list] but literal doesn't support that either 🙈

from flupy.

(Keeping the discussion here, linked the PR)

Ok i think i got it to work, the tricky thing is messing with the default params. One problem is that default params always must follow non-default ones. So for my solution to work, we need to arrange the params in the function signature.
This is backwards incompatible for users who do things such as .collect(5, list). Now they will have to do .collect(list, 5).

I suppose we could do isinstanceof checks to detect if they are passing the parameters in the wrong order. Help them swap it around and issue a warning about this breaking change in the update.

There is another workaround suggest here to fix this at the typing level but i couldn't understand it.

The final alternative is to just define to_set and to_list methods. :D

from flupy.

Nicely done getting that @overload to work!

At this point .collect is so commonly used that a backwards incompatible change to its signature isn't going to be feasible, even with a deprecation cycle.

If we add a new method that does the same thing (e.g. .resolve(container_type)) the n: int arg can be dropped because .take(n).collect(list) == .collect(list, n). That makes the overloads more straightforward, but now that you mention it .to_list() and .to_set() would be more discoverable and readable.

For that reason, I'm leaning towards .to_list()

I'm traveling for the next 2 weeks so it may be a minute before the changes get implemented & docs updated (likely the week of the 25th)

Thanks again for your help, was great to get a second pair of eyes on this!

from flupy.

yeah i do agree its better to just go with to_set and to_list. Happy to help! enjoy travelling!

from flupy.