Skip to content

Conversation

Josephasafg
Copy link
Contributor

No description provided.

@github-actions github-actions bot added feature New functionality size:xl labels Mar 18, 2025
**kwargs,
}
)

Copy link
Contributor

Choose a reason for hiding this comment

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

The below looks like a generic util func that can be used globally in the sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its currently used only for this list function. when we have a second list function that requires similar handling, i'll move it to the http client. but for now I think its okay to leave it here

Copy link
Contributor

@miri-bar miri-bar left a comment

Choose a reason for hiding this comment

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

Great work!
Left a couple of small comments

class Batch(AI21BaseModel):
id: str
"""The ID of the batch."""
status: Literal["FAILED", "PROCESSING", "COMPLETED", "EXPIRED", "CANCELLED", "PENDING"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder - add STALE


ResponseT = TypeVar("_ResponseT", bound=Union["AI21BaseModel", str, httpx.Response, List[Any]])
StreamT = TypeVar("_StreamT", bound=Stream[Any])
AsyncStreamT = TypeVar("_AsyncStreamT", bound=AsyncStream[Any])
Copy link
Contributor

Choose a reason for hiding this comment

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

PaginationT rename suggestion to be extra clear -> SyncPaginationT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

params: Dict[str, Any] | None = None,
response_cls: Type[PageT] | None = None,
**kwargs: Any,
):
Copy link
Contributor

@amirai21 amirai21 Mar 19, 2025

Choose a reason for hiding this comment

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

can we change self.request_method to self.or fetch_items_method (or similar) to make it clear what it does? (the first sounds like its http verb)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops rename was missed. I called it request_callback is that ok with you?

self,
after: str | NotGiven = NOT_GIVEN,
limit: int | NotGiven = NOT_GIVEN,
) -> AsyncPagination[Batch]:
Copy link
Contributor

@amirai21 amirai21 Mar 19, 2025

Choose a reason for hiding this comment

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

worth a docstring that limit will control the page_size of this paginated api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problemos

@github-actions github-actions bot added size:xxl and removed size:xl labels Mar 19, 2025
@github-actions github-actions bot added the lgtm Looks Good to Me label Mar 19, 2025
@Josephasafg Josephasafg merged commit 1d8506e into main Mar 23, 2025
23 of 24 checks passed
@Josephasafg Josephasafg deleted the feat_batch_api branch March 23, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New functionality lgtm Looks Good to Me size:xxl

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants