-
Notifications
You must be signed in to change notification settings - Fork 1
fix: apply review-recommended fixes for 2.3.0 #8
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,6 +239,18 @@ underlying connection when the context is exited. | |
| ```python | ||
| import os | ||
| from youdotcom import You | ||
| from youdotcom.models import ( | ||
| ExpressAgentRunsRequest, | ||
| WebSearchTool, | ||
| ResponseCreated, | ||
| ResponseStarting, | ||
| ResponseOutputItemAdded, | ||
| ResponseOutputContentFull, | ||
| ResponseOutputTextDelta, | ||
| ResponseOutputItemDone, | ||
| ResponseDone, | ||
| ) | ||
| from youdotcom.utils import eventstreaming | ||
|
|
||
|
|
||
| with You( | ||
|
|
@@ -287,7 +299,7 @@ with You( | |
|
|
||
| elif isinstance(event_data, ResponseDone): | ||
| print("\n🎉 Response completed!") | ||
| print(f" Runtime: {event_data.response.run_time_ms} seconds") | ||
| print(f" Runtime: {event_data.response.run_time_ms} ms") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. One small style note: the rest of the streaming example output uses emoji (🔍, ✍️, 🌐, 🎉) which is fine for illustration, but the label should match the field name to avoid confusion. |
||
| print(f" Finished: {event_data.response.finished}") | ||
|
|
||
| else: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,7 @@ | |
| ) | ||
| from .httpclient import AsyncHttpClient, HttpClient | ||
| from .utils import Logger, RetryConfig, remove_suffix | ||
| from dataclasses import dataclass | ||
| from pydantic import Field | ||
| from dataclasses import dataclass, field | ||
| from typing import Callable, Dict, Optional, Tuple, Union | ||
| from youdotcom import models | ||
| from youdotcom.types import OptionalNullable, UNSET | ||
|
|
@@ -36,7 +35,7 @@ class SDKConfiguration: | |
| sdk_version: str = __version__ | ||
| gen_version: str = __gen_version__ | ||
| user_agent: str = __user_agent__ | ||
| retry_config: OptionalNullable[RetryConfig] = Field(default_factory=lambda: UNSET) | ||
| retry_config: OptionalNullable[RetryConfig] = field(default_factory=lambda: UNSET) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct fix. Worth noting: this is in auto-generated source ( |
||
| timeout_ms: Optional[int] = None | ||
|
|
||
| def get_server_details(self) -> Tuple[str, Dict[str, str]]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -145,10 +145,10 @@ def do_request() -> httpx.Response: | |||||||||||||||||
| if res.status_code == parsed_code: | ||||||||||||||||||
| raise TemporaryError(res) | ||||||||||||||||||
| except (httpx.NetworkError, httpx.TimeoutException) as exception: | ||||||||||||||||||
| if retries.config.retry_connection_errors: | ||||||||||||||||||
| raise | ||||||||||||||||||
| if not retries.config.retry_connection_errors: | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good fix — logic was completely inverted. Before: if retry_connection_errors: # True → raise (prevented retry!)
raise
raise PermanentError(...) # False → also prevented retryBoth branches prevented retrying. After this fix, when One minor nit: it would help to add a one-line comment explaining why we re-raise the unwrapped exception here vs. wrapping in
Suggested change
|
||||||||||||||||||
| raise PermanentError(exception) from exception | ||||||||||||||||||
|
|
||||||||||||||||||
| raise PermanentError(exception) from exception | ||||||||||||||||||
| raise | ||||||||||||||||||
| except TemporaryError: | ||||||||||||||||||
| raise | ||||||||||||||||||
| except Exception as exception: | ||||||||||||||||||
|
|
@@ -189,10 +189,10 @@ async def do_request() -> httpx.Response: | |||||||||||||||||
| if res.status_code == parsed_code: | ||||||||||||||||||
| raise TemporaryError(res) | ||||||||||||||||||
| except (httpx.NetworkError, httpx.TimeoutException) as exception: | ||||||||||||||||||
| if retries.config.retry_connection_errors: | ||||||||||||||||||
| raise | ||||||||||||||||||
| if not retries.config.retry_connection_errors: | ||||||||||||||||||
| raise PermanentError(exception) from exception | ||||||||||||||||||
|
|
||||||||||||||||||
| raise PermanentError(exception) from exception | ||||||||||||||||||
| raise | ||||||||||||||||||
| except TemporaryError: | ||||||||||||||||||
| raise | ||||||||||||||||||
| except Exception as exception: | ||||||||||||||||||
|
|
@@ -229,9 +229,6 @@ def retry_with_backoff( | |||||||||||||||||
| except Exception as exception: # pylint: disable=broad-exception-caught | ||||||||||||||||||
| now = round(time.time() * 1000) | ||||||||||||||||||
| if now - start > max_elapsed_time: | ||||||||||||||||||
| if isinstance(exception, TemporaryError): | ||||||||||||||||||
| return exception.response | ||||||||||||||||||
|
|
||||||||||||||||||
| raise | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behavioral change that may surprise callers: max_elapsed_time exhaustion now always raises. Before this change, when all retry time was exhausted and the last error was a After this change, the function unconditionally raises — callers that relied on the old return-on-timeout pattern will now receive an unhandled exception. The new behavior is more correct (returning a failed response silently is bad), but this is a de-facto behavioral breaking change. Consider:
|
||||||||||||||||||
|
|
||||||||||||||||||
| sleep = _get_sleep_interval( | ||||||||||||||||||
|
|
@@ -259,9 +256,6 @@ async def retry_with_backoff_async( | |||||||||||||||||
| except Exception as exception: # pylint: disable=broad-exception-caught | ||||||||||||||||||
| now = round(time.time() * 1000) | ||||||||||||||||||
| if now - start > max_elapsed_time: | ||||||||||||||||||
| if isinstance(exception, TemporaryError): | ||||||||||||||||||
| return exception.response | ||||||||||||||||||
|
|
||||||||||||||||||
| raise | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The async path has the same max_elapsed_time change (see |
||||||||||||||||||
|
|
||||||||||||||||||
| sleep = _get_sleep_interval( | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,7 +144,7 @@ def _parse_security_scheme_value( | |
| elif sub_type == "query": | ||
| query_params[header_name] = [value] | ||
| else: | ||
| raise ValueError("sub type {sub_type} not supported") | ||
| raise ValueError(f"sub type {sub_type} not supported") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good fix. All three Since this is auto-generated code, same note as |
||
| elif scheme_type == "openIdConnect": | ||
| headers[header_name] = _apply_bearer(value) | ||
| elif scheme_type == "oauth2": | ||
|
|
@@ -158,9 +158,9 @@ def _parse_security_scheme_value( | |
| elif sub_type == "custom": | ||
| return | ||
| else: | ||
| raise ValueError("sub type {sub_type} not supported") | ||
| raise ValueError(f"sub type {sub_type} not supported") | ||
| else: | ||
| raise ValueError("scheme type {scheme_type} not supported") | ||
| raise ValueError(f"scheme type {scheme_type} not supported") | ||
|
|
||
|
|
||
| def _apply_bearer(token: str) -> str: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,9 +70,10 @@ def test_research_with_sources(self, server_url, api_key): | |
|
|
||
| assert isinstance(res, ResearchResponse) | ||
| assert res.output is not None | ||
| if res.output.sources: | ||
| for source in res.output.sources: | ||
| assert source.url is not None | ||
| assert res.output.sources is not None | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good strengthening of the assertion. The previous However, these tests depend on the mock server handling
The mock server binary also needs to be rebuilt for Linux (see the separate comment on the binary). |
||
| assert len(res.output.sources) > 0 | ||
| for source in res.output.sources: | ||
| assert source.url is not None | ||
|
|
||
|
|
||
| class TestResearchAsync: | ||
|
|
@@ -96,6 +97,7 @@ async def test_basic_research_async(self, server_url, api_key): | |
| assert isinstance(res, ResearchResponse) | ||
| assert res.output is not None | ||
| assert res.output.content is not None | ||
| assert len(res.output.content) > 0 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consistent with the sources fix above — good to add this. The same mock server gap applies here: |
||
|
|
||
|
|
||
| class TestResearchErrors: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2.3.0 section is useful and well-structured.
Two minor issues to fix:
Broken link to
examples/: Line 331 links toexamples/but onlyexamples/api-example-calls.pyexists (not a directory). Consider changing to[examples/api-example-calls.py](examples/api-example-calls.py)or removing the link.Missing retries behavioral change: The retries fix (
max_elapsed_timeexhaustion now raises instead of returning the failed response) is a behavior change that existing users relying on response-code checking after retries might hit. Worth a brief note in this section.