-
Notifications
You must be signed in to change notification settings - Fork 705
feat: add type-safe array items helper functions #396
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
""" WalkthroughThis change introduces specialized helper functions for defining array item schemas in a more type-specific manner, including functions for string, string enum, number, and boolean types. The generic Items function is enhanced with improved documentation, and comprehensive tests are added to ensure compatibility and correctness of the new helpers. Changes
Possibly related issues
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
mcp/tools.go (3)
948-966
: Docstring omits the newly-addedItemsStringEnum
helperThe comment advertises the three “simple type” helpers but forgets to mention
ItemsStringEnum
. Being the most discoverable place for users, the omission will make the enum variant harder to find.-// For simple types, use ItemsString(), ItemsNumber(), ItemsBoolean() instead. +// For simple types, use ItemsString(), ItemsStringEnum(), ItemsNumber(), +// and ItemsBoolean() instead.
1010-1023
: Duplicate implementation — consider re-usingItemsString(Enum(...))
ItemsStringEnum
is a thin wrapper that manually reproduces the map literal already produced byItemsString(Enum(values...))
. Re-using the existing helper avoids one code path diverging from the other in the future:-func ItemsStringEnum(values []string) PropertyOption { - return func(schema map[string]any) { - schema["items"] = map[string]any{ - "type": "string", - "enum": values, - } - } -} +func ItemsStringEnum(values []string) PropertyOption { + return ItemsString(Enum(values...)) +}This keeps behaviour identical while removing duplication.
1032-1065
: Four nearly-identical helpers – factor out the boilerplate
ItemsString
,ItemsNumber
, andItemsBoolean
share ~90 % of their code.
A tiny unexported helper would make future maintenance (and linting) easier:func itemsOf(kind string, opts ...PropertyOption) PropertyOption { return func(schema map[string]any) { item := map[string]any{"type": kind} for _, o := range opts { o(item) } schema["items"] = item } } func ItemsString(opts ...PropertyOption) PropertyOption { return itemsOf("string", opts...) } func ItemsNumber(opts ...PropertyOption) PropertyOption { return itemsOf("number", opts...) } func ItemsBoolean(opts ...PropertyOption) PropertyOption { return itemsOf("boolean", opts...) }Smaller surface, one place to apply option filtering (see previous comment), and no behavioural risk.
mcp/tools_test.go (1)
532-714
: Great compatibility coverage – a few additions could harden the testThe test thoroughly checks equivalence of the produced items schemas across helpers – nice!
Two quick wins to make it future-proof:
- Also assert
oldArrayProp["uniqueItems"]
etc. if the array helper ever gains such options – avoids silent regressions.- Add a negative case (e.g. pass an unsupported option like
Required()
toItemsString
and ensure the resulting schema excludes it). This will lock in the intended filtering logic once implemented.Not critical for merge, but easy improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcp/tools.go
(2 hunks)mcp/tools_test.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mcp/tools_test.go (1)
mcp/tools.go (14)
Tool
(472-483)NewTool
(569-591)WithDescription
(612-616)WithArray
(892-910)Description
(671-675)Items
(962-966)ItemsString
(996-1008)ItemsStringEnum
(1016-1023)MinLength
(723-727)MaxLength
(715-719)ItemsNumber
(1032-1044)Min
(759-763)Max
(751-755)ItemsBoolean
(1053-1065)
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.
Thanks for these additions @davidleitw
I'm wondering if ItemsX
is the best names for these functions. Wouldn't WithXItems
be better?
Functionally, since this PR does not break any functionality and improves user experience, we can merge it.
The future proposal to improve the API sounds good to me. Users can still use the old way if they need to do complex stuff, but most common use cases can be captured with the abstractions like you mentioned.
I think that the API needs to be simple and intuitive and if it helps developers build schemas accurately faster, then we can definitely implement this.
Tagging @ezynda3 @rwjblue-glean and @dugenkui03 as well for their inputs.
Hi, @pottekkat Yeah, I can definitely try refactoring the API to use the WithXItems naming pattern instead — that does sound more intuitive. I’ll experiment with it and see how it plays out across the different use cases. Let me know if you have any specific preferences or edge cases you’d like me to consider while adjusting the naming! |
Hi @pottekkat Thank you for the excellent naming suggestion! I've implemented the changes you recommended. 🔄 Changes MadeI've renamed all the Items functions to follow the
📝 What Was Updated
🎯 Current BehaviorThe functions now work exactly the same as before, but with more intuitive naming: // Before
WithArray("tags", ItemsString())
WithArray("scores", ItemsNumber(Min(0), Max(100)))
// After
WithArray("tags", WithStringItems())
WithArray("scores", WithNumberItems(Min(0), Max(100))) ✅ TestingAll tests pass, confirming that:
The new naming is much more intuitive and consistent with the rest of the API. Thank you for the great suggestion! As for the future ItemsObject proposal — if this direction sounds good to everyone, I’d be happy to explore contributing it. I can work on a detailed design document and implementation plan in a separate PR. Open to discussion and feedback from the team! |
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.
Thank you for your quick response. Yes, if you are interested, you can go ahead with the proposal. I will wait for others to comment as well regarding this change and your planned itemsObject proposal.
- Add WithStringItems(), WithNumberItems(), WithBooleanItems() for type-safe array schema construction - Add WithStringEnumItems() for string enum arrays with cleaner API - Improve Items() documentation with examples and usage guidance - Add comprehensive compatibility tests ensuring new APIs generate identical schemas - Use WithXItems naming pattern for consistency with existing API design
feat: add type-safe array items helper functions
Description
This PR introduces type-safe helper functions for configuring array items in MCP tool schemas. The original
Items()
function acceptsany
type, which can lead to runtime errors when developers accidentally pass incompatible types. These new helper functions provide compile-time type safety for common array element types while maintaining full backward compatibility.The new functions include:
ItemsString(opts ...PropertyOption)
- for string arrays with optional constraintsItemsNumber(opts ...PropertyOption)
- for number arrays with optional constraintsItemsBoolean(opts ...PropertyOption)
- for boolean arrays with optional constraintsItemsStringEnum(values []string)
- for string enum arrays with predefined valuesAll functions generate identical JSON schemas to their manual
Items()
equivalents, ensuring seamless migration and compatibility.Type of Change
Checklist
Additional Information
Implementation Scope and Design Decisions
This implementation focuses on simple array types (string, number, boolean) which cover the majority of common use cases. The design maintains the existing functional options pattern for consistency with the broader API.
API Consistency Analysis
During implementation, we identified an interesting API consistency gap: while top-level object properties enjoy full type-safe API support (
WithString
,WithNumber
, etc.), complex object arrays still require manualmap[string]any
construction.For example:
Current Solution Sufficiency
Important: The existing manual construction approach using
Items(map[string]any{...})
remains fully functional and sufficient for all complex scenarios. It provides complete flexibility for any JSON Schema construct without additional learning overhead.Future Enhancement Proposal
Based on the API consistency analysis, I would like to respectfully propose considering a future enhancement to introduce an
ItemsObject
API that could provide type-safe construction for complex object arrays.The approach I have in mind would use a Property constructor pattern:
This would help maintain API consistency between top-level and array item definitions while leveraging existing
WithString
,WithNumber
functions for reusability.However, I fully understand this would introduce additional API complexity, and I want to be mindful of the project's goals for simplicity and maintainability.
Question for maintainers:
Would this direction be worth exploring in a future proposal, or would you prefer to keep the current manual construction approach as the primary solution for complex object arrays?
I'm very open to feedback and happy to discuss the trade-offs between API consistency and simplicity. If this isn't the right direction for the project, I completely understand and respect that decision.
Backward Compatibility
Items()
approach remains the recommended solution for complex object arraysThis implementation strikes a balance between improving developer experience for common cases while maintaining simplicity and full backward compatibility.
Summary by CodeRabbit