KEMBAR78
Remove `update_json_schema` (bad practice func) by sydney-runkle · Pull Request #9125 · pydantic/pydantic · GitHub
Skip to content

Conversation

@sydney-runkle
Copy link
Contributor

@sydney-runkle sydney-runkle commented Mar 27, 2024

Fix #8381

Removing a function that was minimally used, and was following bad practice. This function was problematic in that it both:

  1. Modified an argument in place
  2. Returned said modified argument

This is technically a change (bc the update_json_schema func was public), but I'm not too concerned about this - it was a think wrapper around the some_dict.update function, so it can easily be replaced. I've marked this as relnotes-change accordingly, and will draw attention to this in the release notes.

Selected Reviewer: @alexmojaki

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 27, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 39075f3
Status: ✅  Deploy successful!
Preview URL: https://2f462969.pydantic-docs2.pages.dev
Branch Preview URL: https://remove-old-json-schema-func.pydantic-docs2.pages.dev

View logs

@sydney-runkle
Copy link
Contributor Author

Please review

@sydney-runkle sydney-runkle added the relnotes-change Used for changes to existing functionality which don't have a better categorization. label Mar 27, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 27, 2024

CodSpeed Performance Report

Merging #9125 will not alter performance

Comparing remove_old_json_schema_func (39075f3) with main (b30f44d)

Summary

✅ 13 untouched benchmarks

Copy link
Contributor

@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

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

I think this is OK to merge, but I will point out that @dmontagu and the issue title said to deprecate the function, not remove it.

@sydney-runkle sydney-runkle enabled auto-merge (squash) March 27, 2024 21:02
@sydney-runkle sydney-runkle merged commit 3ad2018 into main Mar 27, 2024
@sydney-runkle sydney-runkle deleted the remove_old_json_schema_func branch March 27, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review relnotes-change Used for changes to existing functionality which don't have a better categorization.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecate update_json_schema function

2 participants