Skip to content

Conversation

@tminich
Copy link
Contributor

@tminich tminich commented Aug 12, 2025

As discussed in #42

If this is fine I can submit an analogous PR for prewk/result.

Comment on lines +61 to +74
* @template U
*
* @param U $optb
* @return T|U
*/
abstract public function unwrapOr($optb);

/**
* Returns the contained value or computes it from a callable.
*
* @param callable(): T $op
* @return T
* @template U
*
* @param callable(): U $op
* @return T|U
Copy link

Choose a reason for hiding this comment

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

This change will make it behave differently from the Rust implementation. It looks like the these methods there return the same type of value as contained in the Option itself. Here are the docs for reference. So I think this should be as it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho it doesn't have to slavishly like Rust, this makes it more flexible. Rust has much better language level constructs to handle Options/Results, so a bit of leeway is, I think, acceptable.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with this reasoning FWIW, the goal of the lib is to deliver value to the PHP language.

@prewk
Copy link
Owner

prewk commented Aug 15, 2025

Unfortunately some of the CI is a bit dusty. Let's ignore that failed job.

Copy link
Owner

@prewk prewk left a comment

Choose a reason for hiding this comment

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

LGTM!

@prewk prewk merged commit 67420c9 into prewk:master Aug 15, 2025
9 of 10 checks passed
@prewk
Copy link
Owner

prewk commented Aug 15, 2025

Please do the result one as well, and I'll make a release afterwards.

@t1sh0o
Copy link

t1sh0o commented Aug 17, 2025

Please do the result one as well, and I'll make a release afterwards.

Here it is prewk/result#43

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