Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duck type the function in map, reduce, etc. #10312

Closed
wants to merge 1 commit into from

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Feb 24, 2015

Now that we have call-overloading, it would be nice to be able to use it in functions like map, reduce, etc. However, call-overloaded objects can be of any type, which makes it seem necessary to duck-type the "function" passed as an argument to map and friends.

The main design criterion in this PR was to avoid method ambiguities, particularly with packages that might overload either the "function type," the "container type," or both. For better or worse, I therefore decided to make it trivial to overload these by making the exported symbols basically not use any kind of typing, and to perform dispatch on the container through a non-exported family of functions with a c on the end.

This is motivated by the existence of call-overloading
@JeffBezanson
Copy link
Sponsor Member

I do agree with the duck typing part. Soon we will want to make Function an abstract type as well.
Beyond that, this solution is hard to love. You can still have ambiguities between packages if some overload map on the function and others on the container. I also think overloading map on the type of function should be quite unusual; clearly the container owns the operation more.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2015

You can still have ambiguities between packages if some overload map on the function and others on the container.

Sure, but this doesn't change that---it's always been that way.

I also think overloading map on the type of function should be quite unusual; clearly the container owns the operation more.

It is, but existing constructs like filter(r::Regex, v) need to be dealt with. Have an alternative suggestion? I'd love it if we didn't need filterc to handle this.

@JeffBezanson
Copy link
Sponsor Member

My solution would be to leave it alone for now :)

I find the filter(::Regex, x) methods very odd. I don't know why there would be a built-in method for filtering the keys of a dictionary by a regex. That seems extremely special-case. Those methods certainly predate call overloading, so maybe we should instead overload call to make regexes act like predicates. Or maybe just remove those methods entirely.

My "textbook" answer to explain the ambiguity of filter(::Regex, ) is that the overloading is being done at the wrong level: this method doesn't exist to replace the code for filter, but to supply a meaning for "calling" a Regex.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2015

My solution would be to leave it alone for now :)

If that means we can expect something soon, I'm all in favor :). But actually soon, not "someday" :).

@timholy timholy closed this Feb 24, 2015
@JeffBezanson
Copy link
Sponsor Member

Not promising anything amazing, I just don't think we should rush to add mapc and filterc. I also highly question those Regex methods.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2015

Well, all that is fair---and I agree with you on the Regex method. How about I explore how many methods need to be deleted to avoid ambiguities without introducing mapc and friends.

@StefanKarpinski
Copy link
Sponsor Member

Not promising anything amazing, I just don't think we should rush to add mapc and filterc. I also highly question those Regex methods.

Jeff, everything you do is amazing.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 24, 2015

Wow, I'm glad you balked. It turns out that Regex method was the only problem spot. See #10314.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants