Skip to content

Add WithXx overloads that take target instance #706

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stephentoub
Copy link
Contributor

Closes #697

cc: @asklar

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

We should also test that adding a static method using these overloads works.

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I looked at the test failures and it appears that this new WithTools<TToolType>(TToolType target, ...) overload is now being preferred over the WithTools(IEnumerable<McpServerTool> tools) if the compile-time type of the tools being passed is something like McpServerTool[] or List<McpServerTool> instead of exactly IEnumerable<McpServerTool>.

@stephentoub
Copy link
Contributor Author

I looked at the test failures and it appears that this new WithTools<TToolType>(TToolType target, ...) overload is now being preferred over the WithTools(IEnumerable<McpServerTool> tools) if the compile-time type of the tools being passed is something like McpServerTool[] or List<McpServerTool> instead of exactly IEnumerable<McpServerTool>.

Ouch. Not sure why I didn't see those failures when I ran tests locally, but yeah, that's an issue. We can't constraint the TToolType, because any type could legitimately have attributed methods on it. Seems like we probably just want to give it a different name.

@halter73
Copy link
Contributor

A different name for is probably the best bet to avoid the ambiguity, but I'm not sure what we'd call it. Maybe WithToolsFromInstance and WithToolsFromFactory? That would imply we aren't adding static tools though.

I wonder if we could raise an error earlier in cases like these, and throw immediately if WithTools<TToolType> on a type didn't add any tools because no methods had the appropriate attribute. This would go for both the existing version that takes just the generic type parameter and the new instance-based version.

I don't think this would make sense for when Assembly or Type objects are passed as runtime parameters, since you might be programmatically adding a bunch of stuff you're not sure will have any handlers, but it would be very strange to register a type using compile-time generic type parameters without any handlers. It's too bad we cannot make this a compile-time error, but I think a runtime error is probably better than nothing.

@halter73
Copy link
Contributor

How would it be to do an as IEnumerable<McpServerTool> cast of the TToolType in the new overload and call the old one?

@stephentoub
Copy link
Contributor Author

How would it be to do an as IEnumerable<McpServerTool> cast of the TToolType in the new overload and call the old one?

I went with this suggestion. Initially it felt wrong to me, but then I started thinking about it as a feature, where the TType gets to be a provider of tools if it chooses to implement that interface. Plus, an arbitrary type really wouldn't have any reason to implement that interface unless it wanted this behavior, I think. So, let's try it.

I didn't address any of the other argument validation questions. We can address that separately if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IMcpServerBuilder WithTools cannot pass parameters to tool constructor
2 participants