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

Allow open(f,filename,...) for f being any callable #24527

Closed
wants to merge 4 commits into from

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Nov 8, 2017

Since Functions are not the only thing that is callable,
they should not be the only things allowed as first arguments.

Though one can always do

open(t -> mycallable(t), "temp.txt")

This broke me using ExpectationStubs.jl.

I don't believe this introduces an ambiguity, so it should be fine.

@ararslan ararslan added needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Nov 8, 2017
@Ismael-VC
Copy link
Contributor

Ismael-VC commented Nov 11, 2017

Couldn't we have another abstract type, supertype of Function called Callable or something like that?

Any -> Callable -> Function

@nalimilan
Copy link
Member

Couldn't we have another abstract type, supertype of Function called Callable or something like that?

We could, but due to the lack of multiple inheritance some types might be unable to be marked as Callable, so that doesn't completely fix the problem.

@oxinabox
Copy link
Contributor Author

oxinabox commented Nov 13, 2017

My tests are going to hit #14747
if it is still occurring

@vtjnash vtjnash closed this Oct 30, 2023
@vtjnash
Copy link
Member

vtjnash commented Oct 30, 2023

Upstream branch seems to have been deleted. It doesn't seem critical (the OP already mentioned the workaround), and using ::Function seems likely to convey intent better than getting a MethodError later when trying to interpret the 2nd arg as a thing to open and the 1st arg as a thing to call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs news A NEWS entry is required for this change needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants