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

Option existential quantifiers #124

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmagaram
Copy link
Contributor

This adds two functions - Option.isSomeAnd and Option.isNoneOr. Full and really good documentation and tests. These are convenience functions that lead to shorter and easier-to-understand code. These correspond to the quantifiers "forAll" and "exists" in mathematics. We have analogous functions on the Array module.

isSomeAnd is like Array.some. Another common name is exists. But I like the parallel usage here.

isNoneOr is like Array.every. Another common name is forAll. But I and other people find this naming a little confusing for empty collections. For every PR I've received money for, I got $1 million dollars. Technically true.

Rust discussion on isSomeAnd: rust-lang/rust#93050
F#: https://fsharp.github.io/fsharp-core-docs/reference/fsharp-core-optionmodule.html

doc improvements

better docs

rename exists to isSomeAnd like Rust

changelog
@jmagaram
Copy link
Contributor Author

How about this @zth @aspeddro @DZakh and anyone else? This is a small part of #85 and based on analogous functions in Rust and the Array module. As long as we have an Option module I think we should make it more complete and convenient. I am happy to get feedback from the community on a larger set of improvements.

@DZakh
Copy link
Contributor

DZakh commented Jul 27, 2023

I agree that since we have Option module, it should be rich and preferred over Null/Nullable, but I'm actually skeptical about the PR.

There are a few reasons why I don't like the proposed functions:

  • It differs from person to person, but it took me a while to figure out what the functions actually do by their name without taking a look at the code
  • There's already mapOr, and I think it should be the only function to stay in the Option module
    • Since everybody uses the map function a lot, mapOr(false, fn) is more readable than isSomeAnd
    • It uses the parsing/piping approach while isSomeAnd and isNoneOr is more like validation restricted to returning only bool
    • I prefer only one way to do things when DX doesn't suffer. A smaller API is a good thing, especially valuable in bigger teams. That's why golang is popular. And I think making your users falling into the pit of success is very important.

So instead, I'd suggest adding mapErrorOr to cover isNoneOr use case.

@jmagaram
Copy link
Contributor Author

jmagaram commented Jul 28, 2023

Thanks for the thoughtful analysis.

As for naming, some alternate names would be exists(fn) and forAll(fn) or every(fn). I prefer the ones I chose because forAll is a bit unintuitive for the empty/none case.

Without isSomeAnd first you’d do a filter followed by an isSome. Without isNoneOr you’d do a filter followed by isNone. Because both of these functions end up using isX, I think the proposed functions are easily discoverable in the intellisense menu. When you use isSome you notice isSomeAnd and realized you could accomplish what you’re trying to do in one API call.

Alternately you could accomplish both with mapOr but I find the resultant code MUCH less intuitive and doesn’t describe my intent very well. This applies to when I am writing it and when I am later looking at the code to figure out what it is doing. This is the key problem these functions solve - making the code match the intent. I think of it as “is x SOMEthing that meets these requirements?” rather than “let me map it to a bool with false for the None case.”

Piping? Don’t understand that argument. If isSome and isNone is useful - functions that return a bool - then the proposed functions are useful too.

I’m also a big fan of falling into the pit of success. I don’t think that means minimalist and only one way to do something. It means finding a good balance. Technically we don’t need mapOr. We could just have map followed by getOr But it is convenient to combine them. I think the same applies with the functions here.

One last thing. There is an Array.some. Why? You could accomplish the same thing with an Array.filter followed by checking if the array is empty. But these are combined into one useful function because it matches the programmers intent. An Option is like an array with a single item.

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.

2 participants