-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Introduce a function to create a standard AsyncClient with options #655
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
Conversation
src/mcp/shared/httpx_utils.py
Outdated
|
||
|
||
def create_mcp_http_client( | ||
headers: dict[str, Any] | None = None, |
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.
headers: dict[str, Any] | None = None, | |
headers: dict[str, str] | None = None, |
ref.: https://github.com/encode/httpx/blob/6c7af967734bafd011164f2a1653abc87905a62b/httpx/_models.py#L139
src/mcp/shared/httpx_utils.py
Outdated
|
||
|
||
def create_mcp_http_client( | ||
headers: dict[str, Any] | None = None, |
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.
headers: dict[str, Any] | None = None, | |
headers: dict[str, str] | None = None, |
ref.: https://github.com/encode/httpx/blob/6c7af967734bafd011164f2a1653abc87905a62b/httpx/_models.py#L139
src/mcp/shared/httpx_utils.py
Outdated
__all__ = ["create_mcp_http_client"] | ||
|
||
|
||
def create_mcp_http_client( |
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.
I don't think this repository follow private/public APIs properly, but maybe it should start...
def create_mcp_http_client( | |
def _create_mcp_http_client( |
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.
We need to use create_mcp_http_client
across different client transport implementations, so making it private would cause Pyright warnings about accessing private members from other modules.
If we want to indicate that the utils module contains internal implementation details, we could consider renaming the file to something like _internal_httpx_utils.py
. This would signal that the module itself is internal while keeping the functions public for cross-module usage.
However, given that this function is in shared/ and genuinely needs to be shared across modules, keeping it public seems more appropriate. Happy to change it if you prefer the internal module approach though or I'm missing something very obvious.
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.
We need to use create_mcp_http_client across different client transport implementations, so making it private would cause Pyright warnings about accessing private members from other modules.
Sorry, I meant making the module private _httpx_utils.py
, and keeping create_mcp_http_client
.
If we want to indicate that the utils module contains internal implementation details, we could consider renaming the file to something like _internal_httpx_utils.py. This would signal that the module itself is internal while keeping the functions public for cross-module usage.
Exactly.
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.
I'm not strong, since the rest here doesn't follow it.
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.
cool, thank you! Applied the change. Agree, we need to start somewhere
Request as a follow up for a few PRs now, for example #284 and Streamable Http PRs
Created
create_mcp_http_client()
that:follow_redirects=True
by default (consistent with existing usage)