Skip to content
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

Merge release 2.1.4 into 3.0.x #381

Merged
merged 10 commits into from
Oct 3, 2023
Merged

Conversation

github-actions[bot]
Copy link

@github-actions github-actions bot commented Oct 3, 2023

Release Notes for 2.1.4

2.1.x bugfix release (patch)

2.1.4

  • Total issues resolved: 0
  • Total pull requests resolved: 4
  • Total contributors: 3

Static Analysis

Improvement

derrabus and others added 5 commits July 24, 2023 13:42
Currently when partitioning a Collection-object it will return an array
of ReadableCollections unless that return-type is overwritten in the
implementation.

This change makes sure that any implementation of a Collection actually
returns an array of Collections instead.
2.1.x bugfix release (patch)

- Total issues resolved: **0**
- Total pull requests resolved: **4**
- Total contributors: **3**

 - [380: Improve return type for Collection::partition](#380) thanks to @heiglandreas
 - [379: Make returntypes consistent with implementation](#379) thanks to @heiglandreas

 - [377: Add doctrine-project.json to .gitattributes](#377) thanks to @VincentLanglet

 - [376: Doctrine Coding Standard 12](#376) thanks to @derrabus

* tag '2.1.4':
  Improve return type for Collection::partition (#380)
  Make returntypes consistend with implementation (#379)
  Add doctrine-project.json to .gitattributes (#377)
  Doctrine Coding Standard 12 (#376)
@greg0ire
Copy link
Member

greg0ire commented Oct 3, 2023

@derrabus @heiglandreas I've tried fixing the cs issue by adding {@inheritDoc}, but then phpcs complains about that annotation being useless. I have yet to figure out why this happens only when merging up.

@derrabus
Copy link
Member

derrabus commented Oct 3, 2023

This is a bug in the Slevotmat CS. Pin it to the same version we use in DBAL.

@greg0ire
Copy link
Member

greg0ire commented Oct 3, 2023

Just pushed a commit showing that pinning does not seem to fix the issue (I did add annotations). Interestingly, other classes extending Collection such as ArrayCollection don't have the issue, because they override the return annotations:

/**
* {@inheritDoc}
*
* @return static
* @psalm-return static<TKey,T>
*/
public function filter(Closure $p): Collection
{
return $this->createFrom(array_filter($this->elements, $p, ARRAY_FILTER_USE_BOTH));
}

@greg0ire greg0ire force-pushed the 2.1.x-merge-up-into-3.0.x_IPBBMKgx branch 3 times, most recently from 5a0211d to 7fd2179 Compare October 3, 2023 14:51
@greg0ire
Copy link
Member

greg0ire commented Oct 3, 2023

It is more precise, and it makes phpcs happy.

It's actually not more precise, I'll revert to the previous commit 😞

@greg0ire greg0ire force-pushed the 2.1.x-merge-up-into-3.0.x_IPBBMKgx branch from 7fd2179 to 9a86474 Compare October 3, 2023 14:54
@greg0ire
Copy link
Member

greg0ire commented Oct 3, 2023

Cc @kukulich, can you please have look at this?

TL;DR: slevomat/coding-standard@4b2af2f does not seem to cover all cases, we have a case with complex annotations that are not detected as useful to inherit.

@kukulich
Copy link

kukulich commented Oct 3, 2023

@greg0ire I'm not able to reproduce any bug. And I see only these reports https://github.com/doctrine/collections/pull/381/files#annotation_14617881091 and all seems relevant

@greg0ire
Copy link
Member

greg0ire commented Oct 3, 2023

@kukulich when adding {@inheritDoc}, the report will change, saying {@inheritDoc} is useless. I will push a commit for you to see this.

UPD: done

@kukulich
Copy link

kukulich commented Oct 3, 2023

SlevomatCodingStandard.Commenting.UselessInheritDocComment
Reports documentation comments containing only {@inheritDoc} annotation because inheritance is automatic, and it's not needed to use a special annotation for it.

I would suggest to disable this sniff in this repository. It goes against other sniffs that you use.

This implies disabling the UselessInheritDocComment sniff.
@greg0ire greg0ire force-pushed the 2.1.x-merge-up-into-3.0.x_IPBBMKgx branch from 039fae4 to 448e610 Compare October 3, 2023 15:31
greg0ire added a commit to greg0ire/collections that referenced this pull request Oct 3, 2023
According to a maintainer of slevomat/coding-standard, it goes against
other sniffs we use in this repository.

See doctrine#381 (comment)
According to a maintainer of slevomat/coding-standard, it goes against
other sniffs we use in this repository.

See #381 (comment)
Add comment explaining exclusion rule
@greg0ire greg0ire merged commit f917d37 into 3.0.x Oct 3, 2023
9 checks passed
@greg0ire greg0ire deleted the 2.1.x-merge-up-into-3.0.x_IPBBMKgx branch October 3, 2023 20:27
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.

5 participants