-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(tools):exposed configurable parameter as property in McpToolset #4271
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
base: main
Are you sure you want to change the base?
Changes from all commits
63f9008
b25a373
701cacb
0fa2a91
4966b2d
34742e2
a055262
3d48401
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 |
|---|---|---|
|
|
@@ -226,6 +226,37 @@ def _get_auth_headers(self) -> Optional[Dict[str, str]]: | |
|
|
||
| return headers | ||
|
|
||
| @property | ||
| def connection_params(self) -> Union[ | ||
| StdioServerParameters, | ||
| StdioConnectionParams, | ||
| SseConnectionParams, | ||
| StreamableHTTPConnectionParams, | ||
| ]: | ||
| return self._connection_params | ||
|
|
||
| @property | ||
| def auth_scheme(self) -> Optional[AuthScheme]: | ||
| return self._auth_scheme | ||
|
|
||
| @property | ||
| def auth_credential(self) -> Optional[AuthCredential]: | ||
| return self._auth_credential | ||
|
|
||
| @property | ||
| def require_confirmation(self) -> Union[bool, Callable[..., bool]]: | ||
| return self._require_confirmation | ||
|
|
||
| @property | ||
| def header_provider( | ||
| self, | ||
| ) -> Optional[Callable[[ReadonlyContext], Dict[str, str]]]: | ||
|
Comment on lines
+251
to
+253
Contributor
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. |
||
| return self._header_provider | ||
|
|
||
| @property | ||
| def errlog(self) -> TextIO: | ||
| return self._errlog | ||
|
|
||
| async def _execute_with_session( | ||
| self, | ||
| coroutine_func: Callable[[Any], Awaitable[T]], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,61 @@ def test_init_basic(self): | |
| assert toolset._auth_scheme is None | ||
| assert toolset._auth_credential is None | ||
|
|
||
| def test_connection_params(self): | ||
| """Test getting connection params.""" | ||
| toolset = MCPToolset(connection_params=self.mock_stdio_params) | ||
| assert toolset.connection_params == self.mock_stdio_params | ||
|
|
||
| def test_auth_scheme(self): | ||
| """Test getting auth scheme.""" | ||
| toolset = MCPToolset(connection_params=self.mock_stdio_params) | ||
| assert toolset.auth_scheme is None | ||
|
|
||
| def test_auth_credential(self): | ||
| """Test getting auth credential.""" | ||
| toolset = MCPToolset(connection_params=self.mock_stdio_params) | ||
| assert toolset.auth_credential is None | ||
|
|
||
| def test_error_log(self): | ||
| """Test getting error log.""" | ||
| toolset = MCPToolset(connection_params=self.mock_stdio_params) | ||
| assert toolset.errlog == sys.stderr | ||
|
|
||
| def test_auth_scheme_with_value(self): | ||
| """Test getting auth scheme when provided at initialization.""" | ||
| mock_scheme = Mock() | ||
| toolset = MCPToolset( | ||
| connection_params=self.mock_stdio_params, | ||
| auth_scheme=mock_scheme, | ||
| ) | ||
| assert toolset.auth_scheme == mock_scheme | ||
|
|
||
| def test_require_confirmation(self): | ||
| """Test getting require_confirmation flag.""" | ||
| toolset = MCPToolset( | ||
| connection_params=self.mock_stdio_params, | ||
| require_confirmation=True, | ||
| ) | ||
| assert toolset.require_confirmation is True | ||
|
|
||
| def test_header_provider(self): | ||
| """Test getting header_provider.""" | ||
| mock_header_provider = Mock() | ||
| toolset = MCPToolset( | ||
| connection_params=self.mock_stdio_params, | ||
| header_provider=mock_header_provider, | ||
| ) | ||
| assert toolset.header_provider == mock_header_provider | ||
|
|
||
| def test_auth_credential_with_value(self): | ||
| """Test getting auth credential when provided at initialization.""" | ||
| mock_credential = Mock(spec=AuthCredential) | ||
| toolset = MCPToolset( | ||
| connection_params=self.mock_stdio_params, | ||
| auth_credential=mock_credential, | ||
| ) | ||
| assert toolset.auth_credential == mock_credential | ||
|
Comment on lines
85
to
138
Contributor
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 new tests are a good start, but they don't cover all the newly added properties. The properties
Collaborator
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. Can you also add the additional tests. |
||
|
|
||
| def test_init_with_stdio_connection_params(self): | ||
| """Test initialization with StdioConnectionParams.""" | ||
| stdio_params = StdioConnectionParams( | ||
|
|
||
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 formatting of this property definition is unconventional. It's more standard to keep
selfon the same line as the method name. If the line length is an issue, you can break the type hint across multiple lines for better readability.