Refactor Current operation - Let specify a default value
#205
Conversation
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
from
df658a1
to
d3e95e2
Compare
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
2 times, most recently
from
bfefcea
to
01bcd17
Compare
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
from
01bcd17
to
97ab2c7
Compare
There was a problem hiding this comment.
We also need some new checks that use a default value with current here:
https://github.com/loophp/collection/blob/master/tests/static-analysis/current.php
| get_checkIntElement(Collection::fromIterable([1, 2, 3])->get(1)->current()); | ||
| /** @psalm-suppress PossiblyNullArgument @phpstan-ignore-next-line */ |
There was a problem hiding this comment.
same with all of these, has the behaviour changed?
There was a problem hiding this comment.
I think this slight behaviour change is because of the Psalm issue: vimeo/psalm#6685
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
2 times, most recently
from
08cb578
to
296c496
Compare
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
from
296c496
to
8f3eef0
Compare
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
from
8f3eef0
to
85a1a2f
Compare
src/Contract/Operation/Getable.php
Outdated
| * @param TKey $key | ||
| * @param T|null $default | ||
| * @param V|null $default |
There was a problem hiding this comment.
| * @param V|null $default | |
| * @param V $default |
I'm wondering whether this will be enough to prevent NullArgument in the static analysis checks here: https://github.com/loophp/collection/pull/205/files#diff-a937b0d4bd3a09eb4429e096dacf9b26f2dd67d7c351847aa12020dc44fdabe3R71
the return type would also be @return Collection<TKey, T|V>
It's basically saying that null is included in the template V. Seems to be working fine for the updated find operation: https://github.com/loophp/collection/pull/204/files?diff=split&w=1#diff-24b4b03ac10151d1e45f941532d787d12da924e784810b814dcb366380c2bdbcR26-R33. See the static analysis checks there as well
There was a problem hiding this comment.
Faire enough. TBH, this is something that I'm unsure of. I never know if we have to add this |null or not.
I will follow your advice and remove it. Let me know if it's ok.
There was a problem hiding this comment.
I'll have a look and do some testing in a bit
There was a problem hiding this comment.
I made a commit to remove some of these remaining null: f6e05ee
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
from
ca94605
to
0c5e433
Compare
|
I'll add the suggested tests tomorrow. |
|
Ok I'm done with this one, let me know what you think @AlexandruGG . I removed the |
drupol
force-pushed
the
refactor/current--let-specify-a-default-value
branch
from
f30db13
to
4430daa
Compare
There was a problem hiding this comment.
@drupol thanks for this nice addition; it should be good to go now, but have a look at the last commit I made
|
Excellent. |
This PR:
Currentoperation and allow users to specify a default value when the user index is out of bound. The default value by default isnullto stay backward compatible.Currentclass is used on its own.