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.

Sign up for GitHub

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

Jump to bottom

Make returntypes consistent with implementation #379

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Make returntypes consistent with implementation #379

merged 1 commit into from
Sep 29, 2023

Conversation

heiglandreas
Copy link
Contributor

@heiglandreas heiglandreas commented Sep 28, 2023 •
edited by greg0ire
Loading

The implementation of filter and map are working on a Collection and not on a ReadableCollection. Therefore they will also return a Collection and not just a ReadableCollection.

Closes #372

@greg0ire
Copy link
Member

The implementation of filter and map are working on a Collection and not on a ReadableCollection.

I think what both Psalm and PHPStan are saying here is that you don't actually have a guarantee of that according to the annotations on Collection::filter and Collection::map.

@greg0ire
Copy link
Member

greg0ire commented Sep 28, 2023 •
edited
Loading

I saw your toot about fixup! commits, and while I agree they're great, they're AFAIK not taken into account by Gitl{lab,hub}. Either you will have to rebase interactively, or we will have to squash merge. Let us know what your plan is regarding this.

@heiglandreas
Copy link
Contributor Author

Would be great to support that in Github et al, yeah!

But Inwanted to rebase that locally when I'm happy with it. Should have created a draft 🤦

@derrabus
Copy link
Member

I think, we're going to squash the PR in the end anyway.

@greg0ire
Copy link
Member

Should have created a draft

You can still click "convert to draft" after the PR is created 😉

@greg0ire greg0ire changed the title Make returntypes consistend with implementation Make returntypes consistent with implementation Sep 28, 2023
@greg0ire
Copy link
Member

To be squash merged or squashed. When squashing, I think we should apply the same changes I just applied to the title and description (apart maybe from the one with about using a Github keyword, as it only matters to Github).

@greg0ire
Copy link
Member

Oh and I think the target branch is wrong, it should be 2.2 since this is not a bugfix

@stof
Copy link
Member

stof commented Sep 28, 2023

Note that this allows the behavior to match what we had in 1.7.0 (before extracting ReadableCollection)

@greg0ire
Copy link
Member

Ah yes… then forget about my comment, since it can be considered as a fix for a regression.

@heiglandreas
Make returntypes consistend with implementation
c21e2e1 
The implementation of filter and map are working on a Collection
and not on a ReadableCollection. Therefore they will also return
a Collection and not a ReadableCollection.
@heiglandreas heiglandreas force-pushed the modifyReturnTypeForFilterImplementations branch from 24a2719 to c21e2e1 Compare September 28, 2023 13:24
@heiglandreas
Copy link
Contributor Author

All rebased and squashed so that it now is only one commit

SenseException
SenseException approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression as in "not done on purpose in 2.0", even though the major version would allow such a change. But I don't expect much of a negative impact on this change/fix.

derrabus
derrabus approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Should we apply this change to the partition() method as well?

@heiglandreas heiglandreas mentioned this pull request Sep 29, 2023
Merged
@derrabus derrabus added this to the 2.1.4 milestone Sep 29, 2023
@derrabus derrabus merged commit e695349 into doctrine:2.1.x Sep 29, 2023
@github-actions github-actions bot mentioned this pull request Oct 3, 2023
Merged
derrabus added a commit that referenced this pull request Oct 3, 2023
@derrabus
Merge tag '2.1.4' into 2.1.x-merge-up-into-3.0.x_IPBBMKgx
9a86474 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

@SenseException SenseException SenseException approved these changes

@greg0ire greg0ire greg0ire approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Static Analysis
Projects
None yet
Milestone
2.1.4
Development

Successfully merging this pull request may close these issues.

should the return type of ReadableCollection:filter() & ReadableCollection:map() not be static ?
5 participants
@heiglandreas @greg0ire @derrabus @stof @SenseException