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

[2.0] Add new methods Collection::findFirst and Collection::reduce #252

Merged
merged 4 commits into from
Jun 10, 2021
Merged

[2.0] Add new methods Collection::findFirst and Collection::reduce #252

merged 4 commits into from
Jun 10, 2021

Conversation

VincentLanglet
Copy link
Contributor

This is two methods I find useful for the next major.

Collection::find has a better performance than using Collection::filter then Collection::first.

Collection::reduce is similar to array_reduce.

I create this PR to open the discussion about these methods.

@VincentLanglet VincentLanglet changed the title Add new methods Collection::find and Collection::reduce [2.0] Add new methods Collection::find and Collection::reduce Aug 12, 2020
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

lib/Doctrine/Common/Collections/Collection.php Outdated Show resolved Hide resolved
lib/Doctrine/Common/Collections/Collection.php Outdated Show resolved Hide resolved
@SenseException
Copy link
Member

You should probably add docs at https://github.com/doctrine/collections/blob/1.6.x/docs/en/index.rst

Probably the 2.0 docs if this is for the next major release.

@VincentLanglet
Copy link
Contributor Author

I added the documentation.

docs/en/index.rst Outdated Show resolved Hide resolved
docs/en/index.rst Outdated Show resolved Hide resolved
docs/en/index.rst Outdated Show resolved Hide resolved
docs/en/index.rst Outdated Show resolved Hide resolved
greg0ire
greg0ire previously approved these changes Aug 22, 2020
greg0ire
greg0ire previously approved these changes Aug 22, 2020
SenseException
SenseException previously approved these changes Aug 24, 2020
@VincentLanglet
Copy link
Contributor Author

@ostrolucky Sorry for the delay ; I made the changes you asked :)

@VincentLanglet VincentLanglet changed the title [2.0] Add new methods Collection::find and Collection::reduce [2.0] Add new methods Collection::findFirst and Collection::reduce Jan 9, 2021
*
* @psalm-template TReturn
* @psalm-template TInitial
* @psalm-param Closure():TReturn $func
Copy link
Member

Choose a reason for hiding this comment

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

Parameters are required to specify. Please make sure whole definition works in psalm.dev https://psalm.dev/r/61a6394fe6

@ostrolucky
Copy link
Member

Still not it https://psalm.dev/r/b66b6e6a60

@VincentLanglet
Copy link
Contributor Author

Still not it https://psalm.dev/r/b66b6e6a60

I think your test is wrong since you're passing string to array_reduce. @ostrolucky

See https://psalm.dev/r/6ca84608e3

ostrolucky
ostrolucky previously approved these changes Jan 13, 2021
Base automatically changed from master to 2.0.x January 19, 2021 07:24
@VincentLanglet
Copy link
Contributor Author

Is there anything missing to be merged ?

@ostrolucky
Copy link
Member

I don't think so. I am not sure who is responsible for doctrine/collections though.

@greg0ire greg0ire merged commit 534712a into doctrine:2.0.x Jun 10, 2021
@greg0ire
Copy link
Member

Thanks @VincentLanglet !

@VincentLanglet
Copy link
Contributor Author

Thanks @VincentLanglet !

You're welcome.

By the way, I saw multiple times that creating a release 2.0 seems impossible since the library will have difficulties to migrate from version 1 and 2. But when I looked at the 2.0 branches, the changes are currently:

  • Some method are added to the interface.
  • Some return type hint are added.
    Which means that if you're compatible with the 2.0 version, you're still compatible with the v1.

What about focusing the feature of the v2 on this point ? (Being compatible with the v2 should allow to be compatible with the v1 easily). For instance:

With this strategy, a reachable 2.0 milestone could be created and released ; Wdyt ?

@ostrolucky
Copy link
Member

If 2.0 is created now, it will mean we have to create 3.0 for new features/BC breaks, so that's unlikely. It's not worth creating 2.0 at this point IMHO, there are not that many changes.

@VincentLanglet
Copy link
Contributor Author

If 2.0 is created now, it will mean we have to create 3.0 for new features/BC breaks, so that's unlikely. It's not worth creating 2.0 at this point IMHO, there are not that many changes.

I didn't ask for creating 2.0 now. But for a reachable milestone.
Almost 0 issue are flagged with the milestone https://github.com/doctrine/collections/issues?q=is%3Aissue+milestone%3A2.0.0+is%3Aopen and the only one will break the support of 1.x.

The last major version was in 2013, I'm sure they are some things which can be improved and allow librairies to support both ^1 || ^2.

@VincentLanglet VincentLanglet deleted the addNewMethods branch June 10, 2021 07:43
@greg0ire greg0ire added this to the 2.0.0 milestone Jun 10, 2021
@Brammm
Copy link

Brammm commented Jun 17, 2021

Derp, I was just about to create a PR for a similar findFirst method. Is there any reason this is going in 2.0 and not 1.x?

@VincentLanglet
Copy link
Contributor Author

Derp, I was just about to create a PR for a similar findFirst method. Is there any reason this is going in 2.0 and not 1.x?

Adding a method to an interface is a BC-break.

@Brammm
Copy link

Brammm commented Jun 17, 2021 via email

@fnagel
Copy link

fnagel commented Feb 9, 2023

@VincentLanglet Both methods are not part of the UPGRADE.md file or am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants