Skip to content
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

Fix behavior of _json_schema_to_grammar for schemas with many properties #645

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

hudson-ai
Copy link
Collaborator

The conditional for adding commas to incomplete sets of properties in _json_schema_to_grammar._process_object was incorrect, leading to missing commas in the grammar when the number of properties was too high.

Added a test that fails on the current main branch.

@riedgar-ms please take a look when you can

@riedgar-ms
Copy link
Collaborator

That is embarrassing.... thanks for catching this. While you're at it could you also add an explicit test for an object with a single property, please? It looks like some of the other tests do have such a thing, but an explicit test case would be good to have.

@riedgar-ms riedgar-ms self-requested a review February 21, 2024 13:10
@hudson-ai
Copy link
Collaborator Author

@riedgar-ms done. Let me know if you think it's worth parametrizing these two tests with different "type" fields or if you think that's well-covered by other tests.

And not embarrassing at all... I read that bit of code pretty closely myself and wouldn't have caught the problem without dogfooding this corner of the library.

@riedgar-ms
Copy link
Collaborator

Thank you. I think that this is adequate for now - we do have tests working with nested types and the like, so unless we have a failing case, testing effort is probably better spent elsewhere.

@riedgar-ms riedgar-ms merged commit 9927280 into guidance-ai:main Feb 21, 2024
5 checks passed
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.

None yet

2 participants