Skip to content

Conversation

amitschang
Copy link
Contributor

@amitschang amitschang commented Apr 29, 2024

Change Summary

Adds JSON-Schema handler for ImportString which failed to schematize prior to this change. The schema type of importstring is simply string (which gets imported on validation).

There was a test specifically for an exception for this type, which was changed to validate a model containing an importstring type - there is another test already exercising a bad json schema using the same exception type (see https://github.com/pydantic/pydantic/blob/main/tests/test_types.py#L6155), so there does not look to be loss here.

Note: There could be an opportunity to use a regex validation on the string to match something importable, but I left that out of scope here.

Related issue number

Fix #9327

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable - I think the documentation currently implies json schema should work (by omission that it explicitly should not)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @dmontagu

Copy link

codspeed-hq bot commented Apr 29, 2024

CodSpeed Performance Report

Merging #9344 will not alter performance

Comparing amitschang:importstring-json-schema (5b22613) with main (cff0f37)

Summary

✅ 13 untouched benchmarks

@amitschang
Copy link
Contributor Author

please review, thanks!

@amitschang
Copy link
Contributor Author

Also, I think this needs the relnotes-fix label

@sydney-runkle sydney-runkle added the relnotes-fix Used for bugfixes. label Apr 30, 2024
Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Looks great! I've added one small note + I'll merge that suggested change.

I do think that down the line we could add a regex here, but this is a great start!

Thanks for your contribution! 🚀

@sydney-runkle sydney-runkle enabled auto-merge (squash) April 30, 2024 19:27
@sydney-runkle sydney-runkle merged commit 131cfa7 into pydantic:main Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 participants