Skip to content

feat: support required fields (make the others optional) #17

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

Merged
merged 2 commits into from
Apr 8, 2025

Conversation

evacchi
Copy link
Contributor

@evacchi evacchi commented Feb 19, 2025

  • adds isZigOptional(schema, property), checking whether the field is nullable OR it is not declared as required in the parent schema.

requires dylibso/xtp-bindgen-test#28

Result, e.g.:

    CallToolResult:
      required:
        - content
      properties:
        content:
          type: array
          items:
            $ref: "#/components/schemas/Content"
        isError:
          type: boolean
          description: |-
            Whether the tool call ended in an error.

            If not set, this is assumed to be false (the call was successful).
pub const CallToolResult = struct {
    content: []Content,
    /// Whether the tool call ended in an error.
    ///
    /// If not set, this is assumed to be false (the call was successful).
    isError: ?bool = null,

...
}

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi evacchi requested review from bhelx and nilslice February 19, 2025 15:39
@evacchi
Copy link
Contributor Author

evacchi commented Feb 20, 2025

related dylibso/mcp.run-servlets#59

Copy link
Contributor

@bhelx bhelx left a comment

Choose a reason for hiding this comment

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

This looks correct to me but maybe let's have @nilslice chime in

@evacchi
Copy link
Contributor Author

evacchi commented Feb 26, 2025

IIRC @nilslice mentioned we'll need to release https://github.com/dylibso/xtp-bindgen-test/ for the CI to pass

@nilslice
Copy link
Member

oh yea, feel free to release the dependent library and re-run these tests. Don't let me be a blocker on these, if the tests pass we can push it along

@evacchi
Copy link
Contributor Author

evacchi commented Mar 18, 2025

it looks like the test is failing

xtp plugin test $PLUGIN_NAME/zig-out/bin/plugin.wasm --with test.wasm --mock-host mock.wasm

either the test or the mock... 🤔

@nilslice
Copy link
Member

nilslice commented Apr 7, 2025

what's the status here? I keep getting reminders but don't know who's on this now (re: test suite etc)

@evacchi
Copy link
Contributor Author

evacchi commented Apr 7, 2025

it's still me I guess

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi evacchi force-pushed the non-required-fields branch from 55a39ec to 4418c74 Compare April 7, 2025 18:46
@evacchi
Copy link
Contributor Author

evacchi commented Apr 7, 2025

ok making some progress here

  1. we upgraded the JS PDK and the old version was failing with that odd error
  2. now all test assertions are failing, but from the logs I can tell they should pass, so I'd say another JS-PDK-related bug

@nilslice
Copy link
Member

nilslice commented Apr 7, 2025

@evacchi - ok thanks, can you be sure to link this issue to the GitHub ticket to track the JS PDK bug with whatever info you have on it?

@evacchi
Copy link
Contributor Author

evacchi commented Apr 8, 2025

@zshipko fixed the issue, we can merge this and close

@evacchi evacchi merged commit 2c58c32 into main Apr 8, 2025
1 check passed
@evacchi evacchi deleted the non-required-fields branch April 8, 2025 07:21
@evacchi evacchi restored the non-required-fields branch May 15, 2025 11:08
@evacchi evacchi deleted the non-required-fields branch May 15, 2025 11:10
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.

3 participants