Improve tool decorator ergonomics#163
Conversation
|
|
||
|
|
||
| def test_tool_decorator_result_type_is_correct() -> None: | ||
| tool_one = tool("tool_one") |
There was a problem hiding this comment.
hmmm, I am not sure to understand this use case.
What is supposed to happen in this case?
The tool is not invocable really is it?
There was a problem hiding this comment.
I'm preserving whatever semantics already existed, the general use-case is to use it to rename a tool. I added more comments to the tests to clarify things.
| ) -> Callable[[Callable[..., Any]], ServerTool]: ... | ||
|
|
||
|
|
||
| def tool( |
There was a problem hiding this comment.
This improves the named decorator/wrapper overloads, but it regresses the original case:
@tool(description_mode=DescriptionMode.ONLY_DOCSTRING)
def simple_tool() -> None:
"""simple tool"""Because func_or_name is now required, this raises before tool() can return the decorator partial. The same applies to documented forms like
@tool(description_mode="only_docstring"), @tool(output_descriptors=[...]), and @tool(requires_confirmation=True).
Can we restore the zero-positional decorator shape in both typing and runtime? It should be possible to:
- add a keyword-only overload returning
Callable[[Callable[..., Any]], ServerTool] - change the implementation signature to allow
func_or_name: Callable[..., Any] | str | None = None - add a
func_or_name is Nonebranch that returns the no-name partial decorator
One extra typing detail: the overload should accept both the DescriptionMode.* enum values and the documented string literal values ("infer_from_signature", "only_docstring", "extract_from_docstring"), otherwise the docs’ examples still produce type-checker false positives.
There was a problem hiding this comment.
Sorry for the oversight, I've restored the zero-positional decorator shape and made sure it worked at runtime and typing time. I also added the overload to the description mode with a DescriptionModeOptions type alias.
| assert_type(tool_two, ServerTool) | ||
| assert isinstance(tool_two, ServerTool) | ||
|
|
||
| # Decorator with description mode |
There was a problem hiding this comment.
This test uses @tool("tool_three", description_mode=...), so it doesn’t cover the motivating call shape from the PR description, does it?
Can we add regression coverage for the keyword-only decorator form?
@tool(description_mode=DescriptionMode.ONLY_DOCSTRING)
def simple_tool() -> None:
"""simple tool"""Please assert both the runtime result and the type result, e.g. assert_type(simple_tool, ServerTool) and isinstance(simple_tool, ServerTool). I’d also include the documented string-literal form (description_mode="only_docstring") plus small cases for output_descriptors and requires_confirmation, since those all use the same zero-positional decorator path.
There was a problem hiding this comment.
I've added test cases for these options 👍
Using the tool decorator is not very ergonomic due to how the function signature is defined.
This PR adds overloads and tests to ensure that using it doesn't yield false positives while typechecking.