-
Notifications
You must be signed in to change notification settings - Fork 200
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
Support dict_items
for list like fields
#225
Conversation
Codecov Report
@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage 98.66% 98.66%
=======================================
Files 48 48
Lines 5031 5031
Branches 35 35
=======================================
Hits 4964 4964
Misses 67 67
Continue to review full report at Codecov.
|
7e930ff
to
1ef2f6d
Compare
I wasn't sure how to write a test to validate the |
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 this is looking good.
f1c1c93
to
149c3bd
Compare
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 LGTM.
tests/validators/test_dict.py
Outdated
@@ -216,3 +217,58 @@ def test_dict_length_constraints(kwargs: Dict[str, Any], input_value, expected): | |||
v.validate_python(input_value) | |||
else: | |||
assert v.validate_python(input_value) == expected | |||
|
|||
|
|||
@pytest.mark.skipif(platform.python_implementation() == 'PyPy', reason='dict views not implemented in pyo3 for pypy') |
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.
Sorry, but it'll be confusing if a test for list, tuple, set, frozen set is in the file for dict tests.
Please can you copy this to each of those files and simplify the parameterisation accordingly.
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.
No worries. I thought it would be easier to consolidate them all in a single test, but totally happy to make this change.
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 understand, but still think better in this case.
149c3bd
to
1b5010f
Compare
Sadly lots of conflicts :-(. Let me know if you want a hand resolving them. |
Closes 211 This is a follow up to PR 208, which added support for `dict_keys` and `dict_values`.
1b5010f
to
da7dae0
Compare
Resolved the merge conflicts! |
wait, realizing that I'm missing one test 😅. one sec |
da7dae0
to
96703c1
Compare
This is great, thank you so much. |
Happy to help😁 I herd about the project from the Talk Python To Me podcast and wanted to check it out! |
oh great, well we'd love you to contribute more. There will be a lot going on with pydantic and pydantic-core over the next few months, so it should be a fun project be get involved in. |
I'm definitely interested in taking on some more issues! |
Closes #211
This is a follow up to PR #208, which added support for
dict_keys
anddict_values
.