- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
FIX: validation on model attribute with nested Literal breaks #1364
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1364 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3772 3774 +2
Branches 749 750 +1
=========================================
+ Hits 3772 3774 +2
Continue to review full report at Codecov.
|
Hm, the breaks & the loss of code coverage are from imports from other pydantic modules than the one altered in this PR 🤔 Any hints at how you think they are best resolved appreciated. (I would write a test for |
Hi @DBCerigo ! Well it tries to run your tests without extra dependencies (so without @pytest.mark.skipif(not Literal, reason='typing_extensions not installed')
def test_interval_validation_error(): |
Btw imo the method to retrieve all the values of a def all_literal_values(type_: Type[Any]) -> Tuple[Any, ...]:
"""
This method is used to retrieve all Literal values as
Literal can be used recursively (see https://www.python.org/dev/peps/pep-0586)
e.g. `Literal[Literal[Literal[1, 2, 3], "foo"], 5, None]`
"""
if not is_literal_type(type_):
return type_,
values = literal_values(type_)
return tuple(x for value in values for x in all_literal_values(value)) |
From Github user PrettyWood, see PR pydantic#1364
@PrettyWood thanks for guidance, much appreciated. I've added the test skips, and used your improved implementation, and moved the location of the function to the The only bit I think is still not up to scratch is the location of testing Thanks again. (For my own understanding, what is reasoning for having just the Ubuntu CI run without external deps and thus differ from the osx and windows? Or have I misunderstood why it was just the Ubuntu builds that failed before?) |
@DBCerigo Well I can just tell you what I would have done but @samuelcolvin may think otherwise. I would have done a new file As for the CI, I reckon the main reason is just that it's useless to do all checks possible on every OS. It's quite common to make heavy tests on ubuntu (here we test that everything works with extra dependencies, without, without compiled binaries...for all supported python versions). Don't know if it's relevant to do everything possible on every OS. Checks on mac and windows are already quite complete. Maybe there is another reason though 🤷♂️ |
Cool, as this was my thinking also then I'll go for it and @samuelcolvin can say to correct as required.
Thanks for that. Having now looked at the CI steps in more detail I see that the build is of course tested with deps on ubuntu, but is also tested in those other settings (without deps etc.) only on ubuntu, hence the difference - so all clear to me now :) Last thing just to check; I won't re-jig the commit history for this PR too much, as it looks like all PRs are merged with squash by default. Let me know if this is wrong, or it should have a tidier history in any case. |
@DBCerigo I like to have a clean history to help the reviewers but you are right all PRs are squash and merged. Since this PR is very small the whole diff is enough for a review so no need to worry about the history too much |
@PrettyWood yep - if it was more substantial I would make the commit history proper to enabling stepping through the commits. Thanks for all the help again! |
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.
otherwise looks great, sorry for the very slow response.
From Github user PrettyWood, see PR pydantic#1364
Includes moving corresponding tests.
e577e82
to
6949f5b
Compare
@samuelcolvin thanks for the review. Hopefully good to go this time :) |
Thanks so much. |
* Add tests for nested literals validator * Implement flatten literal in validator * Add test for flatten literal * Add changelog entry * Add test skip markers if not Literal * Refactor: use improved all literals implementation From Github user PrettyWood, see PR pydantic/pydantic#1364 * Add testing for typing module Includes moving corresponding tests. * Remove unnecessary type hint * Move all literals test to test_utils
Change Summary
Nested literals, e.g.
Literal['foo', Literal['bar']]
are legal syntax per PEP 586.mypy
correctly resolves them, as doespydantic.mypy
for model attributes.Nested literals break pydantic model attribute validation at run time. For a comprehensive example case, including
pydantic.mypy
output and runtime error output see example script.This PR implements a fix for this by implementing a function to flatten nested literals down to a a list of their raw values.
Bonus notes:
tuple
cast was so that these changes would be back compatible with the literal tests intest_types.py
.set
(allowed_choices_set
) instead of thetuple
(permitted_choices
) in https://github.com/samuelcolvin/pydantic/blob/aaec3c9c15581d2de31bcda0fbf096638386ee20/pydantic/validators.py#L397 but this would have required altering those literal teststest_types.py
, and I wanted to keep the scope as small as possible.test_types.py
look quite stale though (haveskip
markers, and don't use the proper python version switches used in the rest of pydantic). So maybe it would be worth removing them for these ones in
test_validators.py`? Lmk in if so for any case - happy to make amends as wanted.Checklist
changes/1364-DBCerigo.md
file added describing change