-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Static Analysis Checks: Common Operations (I) #106
Static Analysis Checks: Common Operations (I) #106
Conversation
// VALID failures below -> `current` can return `NULL` if the collection is empty | ||
|
||
/** @psalm-suppress InvalidArgument @phpstan-ignore-next-line */ | ||
contains_checkBool(Collection::fromIterable([1, 2, 3])->contains(2)->current()); | ||
/** @psalm-suppress InvalidArgument @phpstan-ignore-next-line */ | ||
contains_checkBool(Collection::fromIterable(['foo' => 'bar', 'baz' => 'taz'])->contains('bar')->current()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these feel very awkward to me. current()
returns T|null
, with null
being returned when the collection is empty. However, in the chaining contains()->current()
this will never be null
because contains()
will always return a collection with a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess in practice when used like this the analysers will not complain:
if (true === Collection::fromIterable([1, 2, 3])->contains(2)->current()) {
// do stuff
}
but if the comparison is not explicit PHPStan at least will complain because the return type is bool|null
:
if (Collection::fromIterable([1, 2, 3])->contains(2)->current()) {
// do stuff
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drupol I'm not sure I can convince you to make contains
just return bool
😛. But it would be so much easier 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was like that in the early versions of the library then I changed. Maybe we should discuss about that in a dedicated PR ?
$this::fromIterable(['a' => 'b', 'c' => 'd']) | ||
->contains('d') | ||
->shouldIterateAs(['c' => true]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is another unexpected behaviour I discovered from contains
, it doesn't always return a list, but rather keeps the key of the searched element
tasks.psalm.ignore_patterns: | ||
- "/.github/" | ||
- "/.idea/" | ||
- "/build/" | ||
- "/benchmarks/" | ||
- "/node_modules/" | ||
- "/resource/" | ||
- "/spec/" | ||
- "/var/" | ||
- "/vendor/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realised in CI Psalm wasn't running in tests
where the static analysis checks are, so removing that ignore pattern here. I was still running them on local 😄
// @todo: Rename this in next major version. | ||
// We cannot use Match::class because PHP 8 has | ||
// a new "match" function and we cannot use the same name. | ||
// @See https://github.com/loophp/collection/issues/56 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this @todo
was needed here anymore, right? This method works in php 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it's a leftover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good here, thanks !
@drupol let me know if this helps you understand how I approach these static analysis and if it's helpful towards adding one for |
docs/pages/api.rst
Outdated
@@ -448,17 +450,24 @@ Signature: ``Collection::compact(...$values);`` | |||
contains | |||
~~~~~~~~ | |||
|
|||
Check if the collection contains one or more values. | |||
Check if the collection contains one or more values. Note that if multiple values are passed the operation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could copy paste the warning box that explain this? For example, see the DropWhile operation: https://loophp-collection.readthedocs.io/en/latest/pages/api.html#dropwhile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I remembered a similar one! Sure can!
Is this valid across all operations btw? Variadic parameters acting as OR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
And yeah, I'm trying to make all the variadic as OR
indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added now. Had to make a tweak to specify that in this case you cannot use it as a logical AND by calling in succession because the collection will have just a bool
after the first call
Yeah go ahead please, going to merge this right now. |
Thanks again @AlexandruGG !!!! |
@drupol btw the part with the variadic parameter made me think of writing a section in the documentation about Principles of working with Collection, with things like:
I might have others come to mind as well 😄. What do you think? |
It would be a big plus indeed! The more documentation we have, the better it is. There are indeed many behaviors that could be documented. |
Yes! These are definitely the ADN of the library and it worth to have mentions in the README. |
This PR adds more static analysis checks for commonly-used methods, and fixes a few type hints in the process.