Improve Collection Typings + Add strict Operation

[AlexandruGG]

This PR starts a process of improving the types on the operations to make the Collection behave like a single-type one, without enforcing the type restriction at runtime unless the strict operation is used. Based on multiple discussions had in this PR and in #99.

[AlexandruGG]

As an aside, there seem to be some differences between the CI setup and running GrumPHP locally. The failures seen in CI don't happen on local. Do you think it could be due to the fact that Psalm is configured to use a cache in CI? (I think we should disable this btw)

ERROR: UnusedPsalmSuppress - tests/static-analysis/CollectionT/append.php:79:21 - This suppression is never used (see https://psalm.dev/207)
/** @psalm-suppress InvalidScalarArgument @phpstan-ignore-next-line */


ERROR: UnusedPsalmSuppress - tests/static-analysis/CollectionT/append.php:81:21 - This suppression is never used (see https://psalm.dev/207)
/** @psalm-suppress InvalidScalarArgument @phpstan-ignore-next-line */


ERROR: UnusedPsalmSuppress - tests/static-analysis/CollectionT/append.php:96:21 - This suppression is never used (see https://psalm.dev/207)
/** @psalm-suppress InvalidArgument @phpstan-ignore-next-line */```

[drupol]

As an aside, there seem to be some differences between the CI setup and running GrumPHP locally. The failures seen in CI don't happen on local. Do you think it could be due to the fact that Psalm is configured to use a cache in CI? (I think we should disable this btw)

No I don't think this is the case.

However, you can try to disable it by adding:

tasks.psalm.no_cache: true

in the local grumphp file, under the parameters directive.

[AlexandruGG]

As an aside, there seem to be some differences between the CI setup and running GrumPHP locally. The failures seen in CI don't happen on local. Do you think it could be due to the fact that Psalm is configured to use a cache in CI? (I think we should disable this btw)

No I don't think this is the case.

However, you can try to disable it by adding:

tasks.psalm.no_cache: true

in the local grumphp file, under the parameters directive.

You're right looks like disabling the cache didn't fix the issue. I can't explain the difference in the errors reported but I've reverted the Psalm config change to report unused psalm-suppress annotations which was causing the issue. I'll probably just use it in local development.

[github-actions]

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

[drupol]

Hi Alex,

I reviewed locally your pull request. I can't wait to merge this :)

However I have a few remarks:

• I think it would be better in this case to create an Operation instead of the TypedIterator. The strict should follow the same pattern as the other operations, at least for version ^4.
• I understand the feeling of having a "already made" typed collection class, but I think we should not. If we want to achieve such things, we should add the documentation and explain how to create it's own class TypedCollection extending Collection.
  I actually do not like the idea of starting to pass parameters in constructors. Collection should stay very basic and flexible so people can implements whatever they want.

WDYT?

[AlexandruGG]

Hi Alex,

I reviewed locally your pull request. I can't wait to merge this :)

However I have a few remarks:

• I think it would be better in this case to create an Operation instead of the TypedIterator. The strict should follow the same pattern as the other operations, at least for version ^4.
• I understand the feeling of having a "already made" typed collection class, but I think we should not. If we want to achieve such things, we should add the documentation and explain how to create it's own class TypedCollection extending Collection.
  I actually do not like the idea of starting to pass parameters in constructors. Collection should stay very basic and flexible so people can implements whatever they want.

WDYT?

Hey @drupol , thanks for this.

• regarding the Operation instead of TypedIterator I can try to refactor this. Just to clarify, this means that we still use the TypedIterator but it gets moved to an operation, similar to how the RandomIterator is used in Shuffle, right?

• I understand the concern regarding passing parameters. I'm personally not a fan of allowing Collection to be extendable, I think it's best to keep it final like it is now though. I was going to suggest providing a TypedCollection which is very small, just extending Collection and overriding getIterator. However, that's not possible if we want to keep it final. I think for now let's go with the strict operation.

[drupol]

* regarding the `Operation` instead of `TypedIterator` I can try to refactor this. Just to clarify, this means that we still use the `TypedIterator` but it gets moved to an operation, similar to how the `RandomIterator` is used in [Shuffle](https://github.com/loophp/collection/blob/master/src/Operation/Shuffle.php#L37), right?

Yes, this is also a valid option.

* I understand the concern regarding passing parameters. I'm personally not a fan of allowing `Collection` to be extendable, I think it's best to keep it `final` like it is now though. I was going to suggest providing a `TypedCollection` which is very small, just extending `Collection` and overriding `getIterator`. However, that's not possible if we want to keep it final. I think for now let's go with the `strict` operation.

Wait wait wait, it's not because you have the final keyword that its not possible to create another Collection class that extends the functionalities of the original one.

I'm all in favor of using the Composition pattern instead of Inheritance, I found it so much easier and cleaner in every way.

I can provide an example if you want to, but a bit later today.

[drupol]

Here's an extremely basic (and streamlined) example on how to extend the collection: https://gist.github.com/drupol/8b646ec5e1b91d45cf20969b4eabb3af

[AlexandruGG]

@drupol refactored into a Strict operation now + added documentation.

There's an issue with phpcsfixer and phpcs where they are in opposition -> phpcs wants space here new class () {} and phpcsfixer wants no space, like new class() {} 😄

[drupol]

@drupol refactored into a Strict operation now + added documentation.

There's an issue with phpcsfixer and phpcs where they are in opposition -> phpcs wants space here new class () {} and phpcsfixer wants no space, like new class() {} 😄

I think you can get rid of the parenthesis after the class keyword ?

[AlexandruGG]

@drupol refactored into a Strict operation now + added documentation.
There's an issue with phpcsfixer and phpcs where they are in opposition -> phpcs wants space here new class () {} and phpcsfixer wants no space, like new class() {} 😄

I think you can get rid of the parenthesis after the class keyword ?

I tried but you cannot, because phpcsfixer adds it back 😄

[AlexandruGG]

looks like this issue: squizlabs/PHP_CodeSniffer#3200

[drupol]

It has also been reported here: PHP-CS-Fixer/PHP-CS-Fixer#5463

[drupol]

I think you should fix it by doing this: https://gist.github.com/drupol/39a459e29764efdc41cfacb28ee4b751

[drupol]

I added a note on how to extend this library here: https://loophp-collection.readthedocs.io/en/latest/pages/usage.html#extending-collection

[AlexandruGG]

I added a note on how to extend this library here: https://loophp-collection.readthedocs.io/en/latest/pages/usage.html#extending-collection

Nice 👍

[AlexandruGG]

I think you should fix it by doing this: https://gist.github.com/drupol/39a459e29764efdc41cfacb28ee4b751

do you not think it's better to just ignore the docs folder? PHPCSFIXER is running on that anyway and I think it's nicer than seeing the phpcs ignore comments in the docs

[AlexandruGG]

I added a note on how to extend this library here: https://loophp-collection.readthedocs.io/en/latest/pages/usage.html#extending-collection

This also broke PHPStan but I've added an ignore for docs now

[drupol]

I think you should fix it by doing this: https://gist.github.com/drupol/39a459e29764efdc41cfacb28ee4b751

do you not think it's better to just ignore the docs folder? PHPCSFIXER is running on that anyway and I think it's nicer than seeing the phpcs ignore comments in the docs

Ok!

[AlexandruGG]

@drupol let's get this released if you've had the chance to review 🚀

[drupol]

Thanks mate! Very nice stuff :)

[AlexandruGG]

Thanks mate! Very nice stuff :)

Thank you for the help on this, I think we had some good collaboration!