Make value_hint optional to allow constant positional arguments #172
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
| "type": "object", | ||
| "required": [ | ||
| "type", | ||
| "value_hint" |
There was a problem hiding this comment.
value_hint is technically extraneous for 'constant' arguments, but we should definitely have it required otherwise since that's how clients can index and reference those arguments when forming the template
There was a problem hiding this comment.
Could you explain how value_hint is used for this? This sounded kind of like an HTML placeholder value to default an input, instead of a key used for referencing. Or maybe I misunderstand you.
Maybe something like value could be used as a fallback? essentially a positional argument must have either a value or a value_hint?
There was a problem hiding this comment.
value_hint is needed to provide a name for positional arguments in cases where the user needs to input them and the client needs to remember them. Without this we only have the index which is quite subject to change (and breakage) between versions of the package.
essentially a positional argument must have either a value or a value_hint?
Yes, I think updating the schema to reflect this would be okay
There was a problem hiding this comment.
I have updated the schema to reflect this.
<!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> This is one attempt to resolve #169. Instead of adding a new "constant" argument type, I expanded the existing positional one and just dropped the `value_hint` requirement. I am not sure if this has VS Code impact @sandy081, @connor4312. Also, removed unneeded release_date values from the samples. ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> I tested the current server.json samples in the repo against the new (less strict) schema. Of course they passed (aside from an unrelated bug in my previous NuGet sample 🤦). ## Breaking Changes <!-- Will users need to update their code or configurations? --> ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [x] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [x] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions -->
4 participants
Motivation and Context
This is one attempt to resolve #169.
Instead of adding a new "constant" argument type, I expanded the existing positional one and just dropped the
value_hintrequirement. I am not sure if this has VS Code impact @sandy081, @connor4312.Also, removed unneeded release_date values from the samples.
How Has This Been Tested?
I tested the current server.json samples in the repo against the new (less strict) schema. Of course they passed (aside from an unrelated bug in my previous NuGet sample 🤦).
Breaking Changes
Types of changes
Checklist
Additional context