NotificationsYou must be signed in to change notification settings
Fork2.3k
Star26k

Support`Hashable` for json validation#10324

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

sydney-runkle merged 3 commits intomainfromhashable-json

Sep 6, 2024

Merged

Support`Hashable` for json validation#10324

sydney-runkle merged 3 commits intomainfromhashable-json

Sep 6, 2024

Conversation

Copy link

Contributor

sydney-runkle commentedSep 5, 2024•
edited
Loading

Alternative to#10005 based on#10005 (comment)

Fix#9567

Thanks@kc0506 for the test inspo!

hashable for json

9395cc7

sydney-runkle added feature request relnotes-feature and removed feature request labels

Sep 5, 2024

Copy link

cloudflare-workers-and-pagesbot commentedSep 5, 2024•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`418ca00`
Status:	✅ Deploy successful!
Preview URL:	https://71485969.pydantic-docs.pages.dev
Branch Preview URL:	https://hashable-json.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hqbot commentedSep 5, 2024•
edited
Loading

CodSpeed Performance Report

Merging#10324 willnot alter performance

_{Comparinghashable-json (418ca00) withmain (852736c)}

Summary

✅ 50 untouched benchmarks

Copy link

Contributor

github-actionsbot commentedSep 5, 2024•
edited
Loading

Coverage report

This PR does not seem to contain any modification to coverable code.

dmontagu requested changes

Sep 5, 2024

View reviewed changes

Copy link

Contributor

dmontagu left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I think you should add a test for the cases that fail validation from JSON (presumably nested JSON objects/arrays would?). I vaguely recall seeing a version where it would convert arrays to tuples and objects to frozendict; I think we should keep the behavior like this and not do that, but I think it's worth adding a test with a note indicating that it was added to document the current behavior.

Other than that, LGTM