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 aliases priority #6023
🐛 Fix aliases priority #6023
Conversation
Deploying with Cloudflare Pages
|
tests/test_aliases.py
Outdated
@@ -238,6 +238,62 @@ class Child(Parent): | |||
assert [f.serialization_alias for f in Child.model_fields.values()] == ['w_ser_alias', 'X', 'Y', 'Z'] | |||
|
|||
|
|||
def test_aliases_priority(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we break these into separate tests? I don't like redefining the type multiple times in the same test, I got super confused for a minute because I looked halfway down after seeing the definition at the top before I realized the type was getting redefined in each block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also would be annoying to debug if something breaks some of these cases and not others, as it will fail out on the first one rather than letting you see precisely which ones fail.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also use pytest.mark.parametrize
but no need if it's hard to structure the code in a parametric way — I think it's fine to just have a handful of similar-but-different tests if that's easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to parametrize, but this case it didn't look that bad... 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than wanting the mega test split up, this looks good to me
tests/test_aliases.py
Outdated
class Model(BaseModel, alias_generator=str.upper): | ||
x: int = Field(..., serialization_alias='x1') | ||
|
||
# validation_alias should take priority over alias_generator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# validation_alias should take priority over alias_generator | |
# serialization_alias should take priority over alias_generator |
I believe this is a copy-paste error
Change Summary
Fix aliases priority.
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)