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

11.0.0 feature branch #63

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

11.0.0 feature branch #63

wants to merge 3 commits into from

Conversation

DusanKasan
Copy link
Owner

No description provided.

@DusanKasan DusanKasan self-assigned this Mar 9, 2020
@DusanKasan DusanKasan added this to the 11.0.0 milestone Mar 9, 2020
@DusanKasan DusanKasan linked an issue Mar 9, 2020 that may be closed by this pull request
Comment on lines 39 to 40
$fn = $this->factory;
return $fn();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$fn = $this->factory;
return $fn();
return ($this->factory)();

{
$generatorFactory = function () use ($collection, $levelsToFlatten) {
$factory = function () use ($collection, $levelsToFlatten): Generator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes you specify Generator as return type ans sometimes iterable.

Copy link

@davidmpaz davidmpaz left a comment

Choose a reason for hiding this comment

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

Just as an addition i would like to say I did test this branch against and small project of mine where i use extensibly the library and i also have extensive coverage of unit/integration tests.

Project on Symfony6 with php 8.1

Everything runs smoothly.

Thanks in advance for this PR

/**
* @return Traversable<TKey, TVal>
*/
public function getIterator(): Traversable

Choose a reason for hiding this comment

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

Why not to return here also iterable? ... So it keeps consistent with other usages in collection_functions.php for example.

{
$generatorFactory = function () use ($collection) {
$factory = function () use ($collection): Generator {

Choose a reason for hiding this comment

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

Sometimes it is used Generator as return type, other times like in the next change set (function values) it is used iterable. I would suggest use always iterable, for consistency unless Generator is required in the inner function, in which case i would then change all of them to Generator.

foreach ($collection as $key => $value) {
$buffer[] = [$key, $value];
}
$factory = function () use ($collection): Traversable {

Choose a reason for hiding this comment

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

Same here regarding return type iterable

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.

Planned features and changes for 11.0.0
3 participants