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

chore: more specific return type for chunk() method #351

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

rela589n
Copy link
Contributor

Follows from #350

@rela589n rela589n requested a review from drupol as a code owner November 26, 2024 12:05
Copy link

what-the-diff bot commented Nov 26, 2024

PR Summary

  • Modified Return Type in chunk Method
    This PR updates the type of data that the chunk method returns. Previously, it could return a list that might be empty. Now, it ensures that the list returned will always contain at least one item. This adjustment improves the method's reliability and predictability in our system.

@drupol
Copy link
Collaborator

drupol commented Nov 26, 2024

I think there are some new errors to fix.

@rela589n
Copy link
Contributor Author

Do you mean this CI error?

image

Unfortunately I could not get my local environment ready for the development (If I remember correctly, if I try to run tests on the main branch, some of them fail) yet

@rela589n rela589n force-pushed the 350-more-specific-chunk-return-type branch 2 times, most recently from 5f228d2 to 1da16de Compare November 30, 2024 13:25
src/Operation/Chunk.php Show resolved Hide resolved
Comment on lines 55 to 56
/** @var non-empty-list<T> $chunk */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this @var annotation psalm won't be able to infer the fact that chunk is never empty when it is yielded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this didn't help either, because psalm treats T somewhat awkwardly.

Threrefore, I have somewhat rewritten the implementation into following variant so that both psalm and phpstan are satisfied:

$chunk[] = $value;

if (count($chunk) >= $size) {
    ++$chunkIndex;

    yield $chunk;

    $chunk = [];
}

src/Operation/Chunk.php Show resolved Hide resolved
@rela589n rela589n force-pushed the 350-more-specific-chunk-return-type branch from 1da16de to ce28b5a Compare November 30, 2024 13:38
Copy link

github-actions bot commented Dec 6, 2024

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Dec 6, 2024
@rela589n
Copy link
Contributor Author

rela589n commented Dec 6, 2024

@drupol , could you help me with this, since right now I'm not sure what I'm expected to do in order to get this PR merged?

@drupol drupol removed the stale label Dec 6, 2024
@drupol
Copy link
Collaborator

drupol commented Dec 10, 2024

Sorry for the long delay, I'm quite busy with this end of year and I don't have much time left for my open-source projects sadly.

Here, it looks like there are code style stuff to update.
Your changes are also introducing new static analysis issues.

Are you able to run the tests on your local machine so you can fix them locally and commit/push when it's done?

@rela589n
Copy link
Contributor Author

I'm quite busy with this end of year and I don't have much time

It's all right, take your time

Here, it looks like there are code style stuff to update.

I have updated the code so that all checks have passed now.
Once you have some bit of time, please review it.

Thank you,
Yevhen

@drupol drupol enabled auto-merge (squash) December 11, 2024 06:29
Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@drupol drupol merged commit 3cfae9d into loophp:master Dec 11, 2024
15 checks passed
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.

2 participants