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: check only first sublevel for validators with each_item #1991

Merged
merged 3 commits into from Feb 11, 2021

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Oct 11, 2020

Change Summary

When using a validator with each_item, the items are all validated
one by one. But if an item is also iterable the subitems are then
validated because the validator is kept as it is.
Now the validator passed to the items is changed and won't be propagated

Related issue number

closes #1255
closes #1933

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

When using a validator with `each_item`, the items are all validated
one by one. But if the items are also iterable the subitems would then
be validated because the validator would be kept as it is.
Now the validator passed to the items is changed and won't be propagated

closes pydantic#1933
@codecov
Copy link

codecov bot commented Oct 11, 2020

Codecov Report

Merging #1991 (4fad457) into master (13a5c7d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1991   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         4199      4202    +3     
  Branches       854       855    +1     
=========================================
+ Hits          4199      4202    +3     
Impacted Files Coverage Δ
pydantic/fields.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

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

@samuelcolvin
Copy link
Member

I'll reply on the issue.

@samuelcolvin
Copy link
Member

see #1933 (comment), let's update this to add recursive.

@PrettyWood PrettyWood force-pushed the fix/each-item-once branch 2 times, most recently from 3444e15 to ce879ea Compare November 29, 2020 21:49
@PrettyWood
Copy link
Member Author

PrettyWood commented Nov 29, 2020

@samuelcolvin Glad you accepted the proposal! I added the recursive kwarg to ensure the breaking change is easy to fix. But as you can see in my test the behaviour before was really weird: it was not really recursive but more a validation for the "last sublevel" based on the field type!.
Indeed

things: List[Tuple[str, str]]

would trigger the validation for each field of each tuple BUT

things: List[tuple]

would trigger the validation for each tuple (like the new behaviour)

So I think the recursive kwarg is wrong in fact sorry. I'm more in favor of

  • actually drop it and make a real breaking change (but again I think it was a bug)
  • add a kwarg but I would rename recursive to something like last_sublevel or deep or whatever and explain how it works (as the behaviour is really weird)

WDYT?

@samuelcolvin
Copy link
Member

Sorry for the slow reply.

I agree recusive is wrong. Let's drop it and call this a breaking change.

@PrettyWood
Copy link
Member Author

PrettyWood commented Jan 2, 2021

Great glad we agree then! :) I removed the last few commits and just added the "breaking change" note with some basic explanation

@samuelcolvin samuelcolvin merged commit f05bdb7 into pydantic:master Feb 11, 2021
@PrettyWood PrettyWood deleted the fix/each-item-once branch February 11, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants