Skip to content

Conversation

@nunomaduro
Copy link
Member

This pull request makes the with helper generic. The current type definition is not perfect, assuming that the return value from with callback will be same as the given value. Yet, it's better then what we had before. At PHPStan level 5 works perfectly, and PHPStorm, of course, is able to infer the types correctly.

Screenshot 2021-11-15 at 12 05 10

Screenshot 2021-11-15 at 12 06 17

@taylorotwell taylorotwell merged commit 4baf4f7 into master Nov 15, 2021
@taylorotwell taylorotwell deleted the feat/with-helper-generic branch November 15, 2021 14:31
@sebdesign
Copy link
Contributor

sebdesign commented Nov 15, 2021

@nunomaduro If think we should handle the case where the callback returns a different type than the value:

E.g.: https://github.com/laravel/framework/blob/master/src/Illuminate/Log/LogManager.php#L132
E.g.: https://github.com/laravel/framework/blob/master/src/Illuminate/Bus/Batch.php#L171

/**
 * Return the given value, optionally passed through the given callback.
 *
 * @template TValue
 * @template TReturn
 *
 * @param  TValue  $value
 * @param  (callable(TValue): TReturn)|null  $callback
 * @return TValue|TReturn
 */
function with($value, callable $callback = null)
{
    return is_null($callback) ? $value : $callback($value);
}

@sebdesign
Copy link
Contributor

Your tests are asserting that the $user parameter is int|User, while it's User. The return type is int|User which is correct.

@nunomaduro
Copy link
Member Author

@sebdesign I am aware of that. Yet, PHPStan does not support default generics values - meaning that the code you proposed will make a phpstan error. So this pull request does the possible job for now.

@sebdesign
Copy link
Contributor

@nunomaduro thank you for your explanation! Indeed you are right we cannot use default/optional generics, I'm sorry for the noise on this PR.

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.

4 participants