Skip to content

Conversation

Viicos
Copy link
Member

@Viicos Viicos commented Dec 20, 2024

Remove duplicated test and improve test_plain_function_schema_is_invalid
to make use of a real model definition.

Change Summary

Closes #11158.

Note that more cleanups could be applied to the discriminator inference logic. I was able to remove one useless condition check, but we still raise the same error in two different places and with a bit of refactoring we can surely simplify it further.

Related issue number

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

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Dec 20, 2024
@Viicos Viicos force-pushed the discriminated-union-errors branch from d40b72f to 4a83068 Compare December 20, 2024 15:04
Copy link

cloudflare-workers-and-pages bot commented Dec 20, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: bca5c56
Status: ✅  Deploy successful!
Preview URL: https://83047679.pydantic-docs.pages.dev
Branch Preview URL: https://discriminated-union-errors.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hq bot commented Dec 20, 2024

CodSpeed Performance Report

Merging #11161 will not alter performance

Comparing discriminated-union-errors (bca5c56) with main (7485725)

Summary

✅ 46 untouched benchmarks

Remove duplicated test and improve `test_plain_function_schema_is_invalid`
to make use of a real model definition.
@Viicos Viicos force-pushed the discriminated-union-errors branch from 4a83068 to bca5c56 Compare December 20, 2024 17:15
Copy link
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pydantic/_internal
  _discriminated_union.py 385
Project Total  

This report was generated by python-coverage-comment-action

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.

Nice, this seems like a good improvement.

If we raised this in more than 2 places I'd suggest we save the message somewhere and use it through a variable, but I think this is fine for the 2 usage approach.

@Viicos
Copy link
Member Author

Viicos commented Dec 21, 2024

If we raised this in more than 2 places I'd suggest we save the message somewhere and use it through a variable, but I think this is fine for the 2 usage approach.

As I mentioned, a refactor would get rid of the two places, so probably best to go this way in the future.

@Viicos Viicos merged commit aea47de into main Dec 21, 2024
54 checks passed
@Viicos Viicos deleted the discriminated-union-errors branch December 21, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants