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 default value serializing #9066

Merged
merged 3 commits into from Mar 21, 2024

Conversation

NeevCohen
Copy link
Contributor

@NeevCohen NeevCohen commented Mar 20, 2024

Change Summary

Fixing a bug where default values of model fields won't be serialized into the json schema

Related issue number

Fix #8942

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
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @alexmojaki

Copy link

codspeed-hq bot commented Mar 20, 2024

CodSpeed Performance Report

Merging #9066 will not alter performance

Comparing NeevCohen:fix/ipv4-address-parsing (88feca8) with main (a3b7214)

Summary

✅ 10 untouched benchmarks

@NeevCohen
Copy link
Contributor Author

Please review

@sydney-runkle sydney-runkle added the relnotes-fix Used for bugfixes. label Mar 21, 2024
@sydney-runkle
Copy link
Member

Something about this feels a bit fishy to me, but I suppose it's a pretty internal detail, so if we want to change this moving forward, we can.

Going to go ahead and merge given that @davidhewitt approved. Thanks for your review DH, and thanks @NeevCohen for the great work on this!

@sydney-runkle
Copy link
Member

Something about this feels a bit fishy to me, but I suppose it's a pretty internal detail, so if we want to change this moving forward, we can.

I guess we're only doing this for the json schema generation, so it is relatively low stakes.

@sydney-runkle sydney-runkle merged commit a80e034 into pydantic:main Mar 21, 2024
53 of 54 checks passed
@sydney-runkle
Copy link
Member

@NeevCohen,

Great work you've been doing! Ping me if you need any direction or advice for other issues / feature requests, if you're interested!

@NeevCohen
Copy link
Contributor Author

@sydney-runkle
Thanks! I'd love to contribute more! I'll keep looking for new issues and see where I can help!

@NeevCohen NeevCohen deleted the fix/ipv4-address-parsing branch March 21, 2024 20:40
@davidhewitt
Copy link
Contributor

I guess we're only doing this for the json schema generation, so it is relatively low stakes.

For what it's worth that was what I was thinking too. It's probably not exactly right, but it's likely better than what to_jsonable_python was doing alone. I think a more comprehensive wiring up might be a lot of work both here and in core, which wasn't clearly worth the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type IPv4Address not parsed
4 participants