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

Add the only(f, x) method, which allows users to provide a default value or customize the error message #44397

Closed
wants to merge 1 commit into from

Conversation

DilumAluthge
Copy link
Member

Closes #43729

@DilumAluthge DilumAluthge added collections Data structures holding multiple items, e.g. sets iteration Involves iteration or the iteration protocol needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Mar 1, 2022
@simeonschaub
Copy link
Member

I don't think that's what we decided on. I'm pretty sure we said this should behave mostly like get, meaning only(f, x) just returns f() if x doesn't have exactly one element. You can still throw from inside f, but it would support lots of other uses as well.

@tkf
Copy link
Member

tkf commented Mar 2, 2022

Yes, we should just call f. It's almost like the CPS approach I discussed in #43773 (reply in thread) which I think let us write more efficient and composable programs.

Also, as discussed in #43491, I strongly believe we should stop introducing ::Function and ::Callable in the public API call signature.

@DilumAluthge DilumAluthge changed the title Add the only(f::Function, x) method, which allows users to customize the error message Add the only(f::Function, x) method, which allows users to provide a default value or customize the error message Mar 2, 2022
@DilumAluthge DilumAluthge changed the title Add the only(f::Function, x) method, which allows users to provide a default value or customize the error message Add the getonly(f, x) method, which allows users to provide a default value or customize the error message Mar 2, 2022
@DilumAluthge DilumAluthge force-pushed the dpa/only-customize-error-message branch 2 times, most recently from 5bdc044 to 9c1efaa Compare March 2, 2022 21:13
Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to name this getonly? I don't think we have any two-arg methods for only yet, so keeping the name should be fine

base/iterators.jl Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member Author

DilumAluthge commented Mar 2, 2022

Any reason to name this getonly? I don't think we have any two-arg methods for only yet, so keeping the name should be fine

Yeah, it was an intentional change. When I'm back at my computer, I'll write up my motivation for changing the name.

@DilumAluthge
Copy link
Member Author

I have removed the restriction that f be a Function. f can now be of any type.

@DilumAluthge DilumAluthge force-pushed the dpa/only-customize-error-message branch from 122d718 to 156f4c7 Compare March 3, 2022 03:47
@simeonschaub
Copy link
Member

Yeah, it was an intentional change. When I'm back at my computer, I'll write up my motivation for changing the name.

So what was the motivation?

@DilumAluthge
Copy link
Member Author

Whoops I completely forgot to reply.

So, my thinking here is that the semantics of this new method seem sufficiently different from the semantics of the original only function. The original only function essentially had this contract: if the input collection does not have exactly one element, throw an exception. This new function doesn't promise the same behavior.

For example, we currently have getindex(collection, key). If the collection does not have the matching key, then it throws an error. If you want a function that lets you not throw an error if the key does not exist, you don't use getindex, because that's not the purpose of the getindex function. Instead, you use either get(collection, key, default) or get(collection, key) do ... end.

So I would say that getindex versus get is the same dichotomy as we have in only versus getonly in this PR. The semantics of getindex versus get are different, so we have two different functions. Similarly, I believe that the semantics of only versus getonly are different enough that they should not be the same generic function.

@simeonschaub
Copy link
Member

It just seems weird to me to have two functions only and getonly since it's impossible to guess how they differ from just their names. getindex vs get is different since getindex allows a variable number of arguments, so we can't define separate methods for it that take a function used to get the default argument without running into ambiguities.

@ericphanson
Copy link
Contributor

ericphanson commented Mar 4, 2022

Maybe one can think about it like pop!. If there’s no key and you provide a default, it’s used, otherwise there’s an error. Likewise here, if you provide a function then it’s used when there’s not exactly 1 element, otherwise you get an error.

@DilumAluthge DilumAluthge changed the title Add the getonly(f, x) method, which allows users to provide a default value or customize the error message Add the only(f, x) method, which allows users to provide a default value or customize the error message Mar 4, 2022
@tkf
Copy link
Member

tkf commented Mar 4, 2022

FWIW, if we really want to derive composable and efficient error/"missing value" handling from a first principle, I believe CPS approach #43773 (reply in thread) I mentioned above is very attractive. For example, maybe it can be done by implementing Base.CPS.only and deriving Base.only from it 1. If we have a mechanical approach like this to solve the generic issue, we don't really need to discuss function name case-by-case basis and remember the relationships between individual functions or between different signatures/arities. We would then be able to write a combinator like

otherwise(only, x) do _
    f()
end

for arbitrary Base.CPS-compatible functions.

Footnotes

  1. only is special since getonly in this PR is isomorphic to Base.CPS.only because of Iterators.map. But I think having different (qualified) names help in general.

@DilumAluthge
Copy link
Member Author

That sounds like a much better approach to me, instead of having to do this manually for each function of interest.

Want to make a PR showing what the implementation of Base.CPS.only would look like? As well as how Base.only would be defined in terms of Base.CPS.only?

@tkf
Copy link
Member

tkf commented Mar 4, 2022

I guess I can make a quick PR but it's almost surely going to cause discussion (for good reasons!) over months (or even years) so if you want to have it in the next minor bump, the best option is to ignore me 😉

So, I think the first step is to get triage's approval for #43773. But I'm not sure if we have arrived at a set of concrete decision points. For example, I don't think we have properly listed downsides of CPS (e.g., it's tedious and maybe too "infectious" to pass around continuation/effect manually; it's error-prone to enforce the return-as-is invariance manually; other languages go towards more implicit effect system and doing this manually may be too archaic in 2022).

Until then, to avoid bitrotting PR, I think I'll be playing it within a package. I've been wanting to look into CPS in https://github.com/tkf/Try.jl.

@DilumAluthge DilumAluthge force-pushed the dpa/only-customize-error-message branch from 345f970 to 16266fa Compare March 4, 2022 20:41
@DilumAluthge
Copy link
Member Author

I'm in no rush to get this in. I'd rather we come up with a general solution, then we can apply that to other functions (first, last, etc.) instead of having to manually add methods for each function of interest.

@DilumAluthge DilumAluthge force-pushed the dpa/only-customize-error-message branch from 39fd542 to 1cbec28 Compare March 6, 2022 01:02
@DilumAluthge DilumAluthge removed the needs tests Unit tests are required for this change label Mar 6, 2022
@DilumAluthge DilumAluthge force-pushed the dpa/only-customize-error-message branch from 1cbec28 to f1fb37c Compare March 17, 2022 06:53
@tkf
Copy link
Member

tkf commented Apr 25, 2022

I'm suggesting using Union{Ok,Err}-valued functions for this. See #45080

@DilumAluthge DilumAluthge deleted the dpa/only-customize-error-message branch October 2, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets iteration Involves iteration or the iteration protocol needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants