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.

Sign up for GitHub

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

Jump to bottom

changes to some coercions #208

Merged
merged 10 commits into from Aug 3, 2022
Merged

changes to some coercions #208

merged 10 commits into from Aug 3, 2022

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Aug 1, 2022 •
edited

  • stop coercing set / frozenset to list / tuple
  • add dict_key and dict_value
  • add iterators

closes #191

@PrettyWood PrettyWood mentioned this pull request Aug 1, 2022
Closed
@codecov
Copy link

codecov bot commented Aug 1, 2022 •
edited

Codecov Report

Merging #208 (49df207) into main (42a465a) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
- Coverage   98.68%   98.63%   -0.05%     
==========================================
  Files          47       47              
  Lines        4936     4988      +52     
  Branches       34       34              
==========================================
+ Hits         4871     4920      +49     
- Misses         65       68       +3
Impacted Files Coverage Δ
src/errors/kinds.rs 99.15% <ø> (ø)
src/input/mod.rs 100.00% <ø> (ø)
src/input/input_python.rs 98.67% <100.00%> (+0.21%) ⬆️
src/validators/tuple.rs 96.85% <0.00%> (-1.58%) ⬇️
src/input/return_enums.rs 99.34% <0.00%> (-0.66%) ⬇️

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 42a465a...49df207. Read the comment docs.

@PrettyWood PrettyWood force-pushed the coercion-tweaks branch 3 times, most recently from 24b77cd to 8b375e8 Compare August 1, 2022 13:29
@PrettyWood
Copy link
Member Author

@samuelcolvin I opened PyO3/pyo3#2527, which should help to do some checks on dict keys and values.
Question: do we want to support dict_items? I feel like it's way rarer to use them directly but it could make sense to add them for consistency

@samuelcolvin
Copy link
Member

Ye, I saw the PR, I'll comment.

On dict_items, I don't really mind, it definitely doesn't need to be added now, I guess on balance it would be better to support it for consistency.

If we don't do it now, let's create an issue to remember.

@PrettyWood
Copy link
Member Author

PrettyWood commented Aug 1, 2022 •
edited

I won't bother with dict items for the moment. Probably a good first issue.
I'll try to tackle generators in this PR tomorrow though I'm not sure they are properly handled by pyo3. If they are not I may just propose this PR as is

@PrettyWood PrettyWood marked this pull request as ready for review August 1, 2022 21:25
@PrettyWood
Copy link
Member Author

Hi @samuelcolvin. This should be ready for a first review. Reading commit per commit should help.
Feel free to make changes directly on the branch if you want to be fast or even close it if the implementation doesn't suit you.

@PrettyWood
Copy link
Member Author

PrettyWood commented Aug 2, 2022 •
edited

Ok I updated the PR without the as_sequence. I went simple and avoided adding too much boilerplate in GenericListLike: iterators, dict keys and dict values are rarer and they need other macros and stuff to handle Result on iter for example. WDYT?

@samuelcolvin samuelcolvin mentioned this pull request Aug 2, 2022
Closed
samuelcolvin
samuelcolvin reviewed Aug 2, 2022
View reviewed changes
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

I'm working on some extra tests now.

Overall this is looking great.

src/input/input_python.rs Outdated Show resolved Hide resolved
src/input/input_python.rs Outdated
Comment on lines 282 to 290
} else if let Ok(iterator) = self.cast_as::<PyIterator>() {
let tuple = PyTuple::new(self.py(), iterator.iter()?.flatten().collect::<Vec<_>>());
Ok(tuple.into())
} else if let Ok(dict_keys) = self.cast_as::<PyDictKeys>() {
let tuple = PyTuple::new(self.py(), dict_keys.iter()?.flatten().collect::<Vec<_>>());
Ok(tuple.into())
} else if let Ok(dict_values) = self.cast_as::<PyDictValues>() {
let tuple = PyTuple::new(self.py(), dict_values.iter()?.flatten().collect::<Vec<_>>());
Ok(tuple.into())
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we have this lump of code in multiple cases, could you move it into a macro (or parameterised function?) and reuse?

@samuelcolvin
Copy link
Member

@PrettyWood I think I've found an error with generators (I think the error is in pyo3), WDYT?

The following unit test fails:

def test_generator_error():
    def gen(error: bool):
        yield 1
        yield 2
        if error:
            raise RuntimeError('error')
        yield 3

    v = SchemaValidator({'type': 'list', 'items_schema': 'int'})
    assert v.validate_python(gen(False)) == [1, 2, 3]
    with pytest.raises(ValidationError, match='???'):
        v.validate_python(gen(True))

Because v.validate_python(gen(True)) doesn't raise an error, instead the result is [1 ,2].

samuelcolvin
samuelcolvin reviewed Aug 2, 2022
View reviewed changes
src/input/_pyo3_dict.rs Outdated
Comment on lines 30 to 40
/// Represents a Python `dict_items`.
#[cfg(not(PyPy))]
#[repr(transparent)]
pub struct PyDictItems(PyAny);

#[cfg(not(PyPy))]
pyobject_native_type_core!(
PyDictItems,
ffi::PyDictItems_Type,
#checkfunction=ffi::PyDictItems_Check
);
Copy link
Member

Choose a reason for hiding this comment

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

should we remove this as I think it's not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!

@PrettyWood
Copy link
Member Author

End of lunch break! I have meetings this afternoon. I can come back on it late at night. Feel free to make the changes you wish to see if you want to go fast

@samuelcolvin
Copy link
Member

okay, will do, thanks for your help.

@PrettyWood
Copy link
Member Author

@PrettyWood I think I've found an error with generators (I think the error is in pyo3), WDYT?

Very weird. Indeed it seems to be buggy. If the issue on pyo3 is confirmed we can open an issue and one of us can try to fix it on pyo3 side

@samuelcolvin
Copy link
Member

Yup, I'm looking into it.

@PrettyWood
Copy link
Member Author

@samuelcolvin You probably already found the issue but I reckon the flatten is wrong. I should collect() list as
a Result<Vec<_>, PyErr>

@samuelcolvin
Copy link
Member

yes, just found it, fixing now.

@samuelcolvin
Copy link
Member

I mean't "pypy", not "mypy" in the last commit. I think this is ready.

@PrettyWood PrettyWood mentioned this pull request Aug 2, 2022
Closed
@samuelcolvin samuelcolvin merged commit cfd5da7 into pydantic:main Aug 3, 2022
@samuelcolvin
Copy link
Member

thanks @PrettyWood 🙏

@PrettyWood PrettyWood deleted the coercion-tweaks branch August 3, 2022 12:16
@ytmimi ytmimi mentioned this pull request Aug 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changes to list, tuple, set, frozenset coersion
2 participants
@PrettyWood @samuelcolvin