loophp / collection

A (memory) friendly, easy, lazy and modular collection class.
https://loophp-collection.rtfd.io/
MIT License
721 stars 35 forks source link

Improve return types for psalm #321

Closed Matth-- closed 10 months ago

Matth-- commented 10 months ago

Psalm cannot always infer the types when not defining the CollectionInterface<Key, Value> type. This makes sure that psalm is aware of the implemented interface and the Collection class.

This PR:

Still open for debate as it changes the static constructor types.

drupol commented 10 months ago

Psalm 5.16 has been released yesterday and adds many new issues. I guess we will have to add them to the baseline.

drupol commented 10 months ago

Can you please run:

  1. Fix CS: ./vendor/bin/grumphp run --tasks=phpcsfixer
  2. Commit
  3. Push
Matth-- commented 10 months ago

@drupol Missed that one as I haven't used it before 😅 . Will update soon!

drupol commented 10 months ago

Merci ❤️

Matth-- commented 10 months ago

Tried to implement something similar for the CollectionDecorator but now phpstan is complaining. I will need to do more investigation to be able to fix this.

drupol commented 10 months ago

Oh curious, what are the issues?

Matth-- commented 10 months ago

After changing the fromIterable method to this in the CollectionDecorator class (so even changing the static to CollectionInterface

  /**
   * @template UKey
   * @template U
   *
   * @param iterable<UKey, U> $iterable
   *
   * @return CollectionInterface<UKey, U>&static<UKey, U>
   */
  public static function fromIterable(iterable $iterable): CollectionInterface
  {
      return new static(Collection::fromIterable($iterable));
  }

I get the psalm error:

ERROR: MixedArgument
at /path/collection/tests/static-analysis/fromIterableAssociateAll.php:31:11
Argument 1 of checklist cannot be mixed, expecting array<string, int> (see https://psalm.dev/030)
checklist(CollectionDecorator::fromIterable(fromIterableAssociateAll::cases())->associate(
    static fn (int $_, fromIterableAssociateAll $item): int => $item->value,
    static fn (fromIterableAssociateAll $item, int $_): string => $item->name,
)->flip()->all(false));

And phpstan spits out:


 ------ ----------------------------------------------------------------------- 
  Line   tests/static-analysis/fromIterableAssociateAll.php                     
 ------ ----------------------------------------------------------------------- 
  31     Parameter #1 $array of function checklist expects array<string, int>,  
         array<int, fromIterableAssociateAll> given.                            
 ------ ----------------------------------------------------------------------- 
drupol commented 10 months ago

OK fair enough, let's not update CollectionDecorator in this PR then.

Was the closing of the PR intentional ?

Matth-- commented 10 months ago

Nope sorry :)

drupol commented 10 months ago

OK cool ! :) Let me know when it's done for you and I'll take care of the rest.

drupol commented 10 months ago

@Matth-- Is it ready to be merged for you?

Matth-- commented 10 months ago

I would say yes for now. 🚀

I'd like to find the root of the problem when i have some more time. Might have to look at the psalm package then.

drupol commented 10 months ago

OK, let's do that. Let me know if I can do further things to improve this project!

Thanks again for your contribution, keep them coming 👍