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

feat(option): new Option::merge() method #425

Closed
wants to merge 1 commit into from

Conversation

devnix
Copy link
Contributor

@devnix devnix commented Oct 18, 2023

The idea, as shown, is easy.

I propose a versatile merge method to make several operations easier between two Options:

Option\some(1)->merge(Option\some(2), static fn (int $a, int $b) => $a + $b)); // Option<3>
Option\none()->merge(Option\some(2), static fn (int $a, int $b) => $a + $b)); // Option<*never*>
Option\some(1)->merge(Option\none(), static fn (int $a, int $b) => $a + $b)); // Option<*never*>

Option\some('Hello')->merge(
    Option\some('World'), 
    static fn (string $a, string $b) => $a.' '.$b)
); // Option<"Hello World">

This is what triggered this particular need in my case:

$value1 = Option\some(false);
$value2 = Option\some(true);

$value1->merge($value2, static fn (bool $a, bool $b) => $a && $b)->unwrapOr(false); // false

$value1 = Option\some(Money::EUR(100));
$value2 = Option\some(Money::USD(100));

$value1->merge($value2, static fn (Money $a, Money $b) => $a->equals($b))->unwrapOr(false); // false

Probably there is a better name for this method, for sure!

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6562143288

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.999%

Totals Coverage Status
Change from base Build 6487837444: 0.0%
Covered Lines: 4155
Relevant Lines: 4197

💛 - Coveralls

@devnix
Copy link
Contributor Author

devnix commented Oct 18, 2023

I'm unsure about those mutation test errors. Any change in my branch does not cause them, am I right?

@veewee
Copy link
Collaborator

veewee commented Oct 18, 2023

Looks like a nice feature. Haven't jumped in the code yet, but was thinking about the name in line of $option->combineWith($other, $closure). WDYT?

@devnix
Copy link
Contributor Author

devnix commented Oct 18, 2023

I was thinking along the lines of using a short name, join was a candidate I was thinking of too. To me, and/andThen was the nicer options here

@veewee
Copy link
Collaborator

veewee commented Oct 22, 2023

I'm unsure about those mutation test errors. Any change in my branch does not cause them, am I right?

I was looking at the mutation errors in #427. Gonna continue somewhere next week most likely.

@veewee
Copy link
Collaborator

veewee commented Nov 22, 2023

@devnix can you rebase against the next branch to get the github actions green again?
I was thinking about naming it compose() this morning. What do you think about that one?

@veewee veewee added Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Status: Revision Needed At least two people have seen issues in the PR that makes them uneasy. Type: Enhancement Most issues will probably ask for additions or changes. labels Nov 22, 2023
@devnix
Copy link
Contributor Author

devnix commented Dec 6, 2023

Hi @veewee! I've been playing with Rust for the last couple of weeks, and I've found that Option has a zip method that may be useful for this purpose. Instead of requiring a callback to join two Some values into one like my idea, it creates a tuple if both are Some. Check this example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bf6a95e1e06b98093c114a274a6e0fe6

My original proposal would be something like zip_with I guess, which is currently experimental, and equivalent to .zip().map(). I could provide the implementation of both if you are ok with that, but I'm unsure about how to represent this tuple in php/phpdoc. Also, maybe the unzip method would be interesting too?

@veewee
Copy link
Collaborator

veewee commented Dec 8, 2023

Just looked at the rust documentation and that indeed seems the way to go and the naming to use here. Feel free to provide all 3 of those functions.
Type-hinting tuples can be done with array{0: A, 1: B} iirc.

https://psalm.dev/r/06049ede28

@devnix
Copy link
Contributor Author

devnix commented Dec 12, 2023

Superseded by #434

@devnix devnix closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Status: Revision Needed At least two people have seen issues in the PR that makes them uneasy. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants