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 handling of bool literals #783

Merged
merged 1 commit into from
Jul 17, 2023
Merged

🐛 Fix handling of bool literals #783

merged 1 commit into from
Jul 17, 2023

Conversation

lig
Copy link
Contributor

@lig lig commented Jul 17, 2023

Change Summary

Treat bool literals (Literal[False], Literal[True]) as generic py objects instead of ints.

Related issue number

See pydantic/pydantic#6601

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @dmontagu

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #783 (eeba6bd) into main (9be711e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
+ Coverage   93.67%   93.70%   +0.03%     
==========================================
  Files          99       99              
  Lines       14290    14292       +2     
  Branches       25       25              
==========================================
+ Hits        13386    13393       +7     
+ Misses        898      893       -5     
  Partials        6        6              
Impacted Files Coverage Δ
src/serializers/type_serializers/literal.rs 75.53% <100.00%> (+5.96%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9be711e...eeba6bd. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 17, 2023

CodSpeed Performance Report

Merging #783 will improve performances by 11.7%

Comparing lig/6601 (eeba6bd) with main (e3ceb4b)

Summary

🔥 1 improvements
✅ 125 untouched benchmarks

Benchmarks breakdown

Benchmark main lig/6601 Change
🔥 test_small_class_core_model 50.8 µs 45.5 µs +11.7%

@lig lig marked this pull request as ready for review July 17, 2023 10:55
@lig
Copy link
Contributor Author

lig commented Jul 17, 2023

please review

@adriangb
Copy link
Member

Good call, but can we special case them as well actually? I'd think it's a pretty reasonable thing to use as a discriminant so fast pathing it makes sense. I don't think we need any sort of lookup, maybe just an enum with two variants being CombinedSerializers.

@lig
Copy link
Contributor Author

lig commented Jul 17, 2023

I don't see why special casing bools here will be beneficial enough. There is no evidence that this is a common case and there is no special treatment necessary to make it work.

@lig lig requested a review from dmontagu July 17, 2023 11:57
@adriangb
Copy link
Member

If you asked me what I would do I would special case nothing. But we're already special casing primitive types so let's do the same for bools.

@adriangb
Copy link
Member

Apologies, after reviewing the code I see that this is about serialization, not validation. Let me check what happens during validation.

@adriangb
Copy link
Member

Validation works well:

from typing import Literal, Union
from pydantic import BaseModel


class Model(BaseModel):
    x: Literal[0, False]
    y: Union[str, Literal[False]]
    z: Union[Literal[False], str]
    xx: Union[int, Literal[False]]
    yy: Union[Literal[False], int]


print(Model(x=False, y=False, z=False, xx=False, yy=False))
#> x=False y=False z=False xx=False yy=False

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

LGMT my earlier feedback was misguided

@lig
Copy link
Contributor Author

lig commented Jul 17, 2023

thanks!

@lig lig merged commit a3b9788 into main Jul 17, 2023
28 checks passed
@lig lig deleted the lig/6601 branch July 17, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants