Skip to content

Allow slf: PyRef<Self>/PyRefMut<Self> in pymethods#419

Merged
kngwyu merged 6 commits intoPyO3:masterfrom
kngwyu:pyclass-regression
Apr 24, 2019
Merged

Allow slf: PyRef<Self>/PyRefMut<Self> in pymethods#419
kngwyu merged 6 commits intoPyO3:masterfrom
kngwyu:pyclass-regression

Conversation

@kngwyu
Copy link
Member

@kngwyu kngwyu commented Mar 28, 2019

Looks like

#[pymethods]
impl Reader {
    fn get_iter(slf: PyRef<Self>, keys: Py<PyBytes>) -> PyResult<Iter> {
        Ok(Iter {
            reader: slf.into(),
            keys: keys,
            idx: 0,
        })
    }
}

and can be called like usual methods from python.
Resolves #356 (comment)
Currently only slf: PyRef is tested.
Another concern is I observed an undesirable segfault when using arg: PyRef<PyBytes> in pymethod(though I'm not sure it's related to this PR

@konstin
Copy link
Member

konstin commented Mar 28, 2019

In general for the self type I think it's better if we had a trait (or maybe reuse extract) that is implemented for all three types instead of trying to guess the right method based on the string-representation of the type

@kngwyu kngwyu changed the title Allow slf: Py<Self>/PyRef<Self>/PyRefMut<Self> in pymethods Allow slf: Py<Self>/PyRef<Self> in pymethods Mar 29, 2019
@kngwyu kngwyu changed the title Allow slf: Py<Self>/PyRef<Self> in pymethods Allow slf: PyRef<Self>/PyRefMut<Self> in pymethods Mar 29, 2019
@kngwyu kngwyu marked this pull request as ready for review March 29, 2019 09:58
@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #419 into master will decrease coverage by 0.06%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   88.06%   87.99%   -0.07%     
==========================================
  Files          83       65      -18     
  Lines        3593     3314     -279     
==========================================
- Hits         3164     2916     -248     
+ Misses        429      398      -31
Impacted Files Coverage Δ
src/lib.rs 0% <ø> (ø) ⬆️
src/instance.rs 96.77% <100%> (+0.32%) ⬆️
src/python.rs 93.58% <100%> (+2%) ⬆️
src/conversion.rs 96.46% <93.93%> (-1.04%) ⬇️
src/class/number.rs 94.2% <0%> (-0.82%) ⬇️
src/type_object.rs 88.4% <0%> (-0.64%) ⬇️
src/class/sequence.rs 96.42% <0%> (-0.59%) ⬇️
src/class/buffer.rs 100% <0%> (ø) ⬆️
src/ffi2/setobject.rs
... and 22 more

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 bc7776e...031f14e. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #419 into master will increase coverage by 0.16%.
The diff coverage is 96.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   87.66%   87.82%   +0.16%     
==========================================
  Files          63       64       +1     
  Lines        3355     3385      +30     
==========================================
+ Hits         2941     2973      +32     
+ Misses        414      412       -2
Impacted Files Coverage Δ
src/lib.rs 0% <ø> (ø) ⬆️
src/python.rs 93.58% <100%> (+2%) ⬆️
src/instance.rs 96.83% <100%> (+0.38%) ⬆️
src/conversion.rs 96.46% <93.93%> (-1.04%) ⬇️
src/ffi3/bytesobject.rs 100% <0%> (ø)
src/types/string.rs 89.83% <0%> (+1.15%) ⬆️

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 f8bf258...c7d6c48. Read the comment docs.

@kngwyu
Copy link
Member Author

kngwyu commented Mar 29, 2019

I think it's better if we had a trait

Experimentally I introduced a new trait named FromPyPointer, which registers the pointer to GIL pool and returns the reference of it.
If you come up with a better design, please let me know.

@konstin
Copy link
Member

konstin commented Mar 30, 2019

I'll have to check if we already have the traits that can do what FromPyPointer does or with we can replace some other code with the new FromPyPointer, but otherwise it's looking good

@kngwyu kngwyu force-pushed the pyclass-regression branch from 43349fa to 9d01253 Compare April 3, 2019 09:03
@kngwyu kngwyu force-pushed the pyclass-regression branch from 9d01253 to ee6deca Compare April 17, 2019 10:05
@kngwyu
Copy link
Member Author

kngwyu commented Apr 22, 2019

@konstin
I want this PR move forward.
Though FromPyPointer is not implemented for Py and PyObject, we should fix this after #446.
It remove lots of Py::from_owned_ptr*s.

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

LGTM

@konstin
Copy link
Member

konstin commented Apr 23, 2019

Merging master should make travis ci pass again (or at least only fail for minimum nightly)

@kngwyu kngwyu force-pushed the pyclass-regression branch from ee6deca to c7d6c48 Compare April 23, 2019 14:16
@kngwyu
Copy link
Member Author

kngwyu commented Apr 24, 2019

Now CI passed, thanks

@kngwyu kngwyu merged commit 60cd0d0 into PyO3:master Apr 24, 2019
@kngwyu kngwyu deleted the pyclass-regression branch April 24, 2019 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Confusing APIs around PyObject

2 participants