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

Refactor paginatorToArray to use AsyncIterable instead of AsyncIterableIterator #745

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Oct 1, 2024

This pull request refactors the paginatorToArray function to use AsyncIterable instead of AsyncIterableIterator. The change simplifies the code and improves readability.


  • The function signature of paginatorToArray has been modified. 🔄
  • Instead of accepting AsyncIterableIterator<T> as its first argument, it now accepts AsyncIterable<T>.
  • This implies that the changes are not user-facing as they are not in the mentioned public API files.ℹ️
  • The developer might be looking to handle sequences of asynchronous operations more flexibly or efficiently. 🚀
  • Note that, although the change is subtle, it might have a vast impact on the function's usage and may affect the codebase that relies on this method. 🎯

generated by pr-describe

Copy link

github-actions bot commented Oct 1, 2024

The change in the GIT_DIFF modifies the type of the iterator parameter in the paginatorToArray function from AsyncIterableIterator<T> to AsyncIterable<T>.

This change should not cause any functional issues if the usage of this function only requires an AsyncIterable and not the extra methods provided by AsyncIterableIterator such as next(). The TypeScript compiler will ensure that the type is used correctly across all usage instances.

However, if the next() method or other Iterator methods are used on iterator in the body of the paginatorToArray function or in the iteratorItem callback function, this change would cause a functional issue. The AsyncIterable interface does not guarantee the presence of these methods.

Without inspecting the function bodies of paginatorToArray and iteratorItem, it's not possible to provide a definitive assessment.

LGTM if no Iterator methods are used on iterator. 🚀 Otherwise, revert the change:

diff --git a/packages/core/src/github.ts b/packages/core/src/github.ts
index b509f7b8..23ebd691 100644
--- a/packages/core/src/github.ts
+++ b/packages/core/src/github.ts
@@ -412,7 +412,7 @@ export async function githubCreatePullRequestReviews(
 }
 
 async function paginatorToArray<T, R>(
-    iterator: AsyncIterable<T>,
+    iterator: AsyncIterableIterator<T>,
     count: number,
     iteratorItem: (item: T) => R[],
     elementFilter?: (item: R) => boolean

generated by pr-review

@pelikhan pelikhan merged commit 963e243 into main Oct 1, 2024
11 checks passed
@pelikhan pelikhan deleted the usage branch October 1, 2024 20:25
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.

1 participant