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

Change the initializer style #344

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Change the initializer style #344

merged 1 commit into from
Feb 13, 2019

Conversation

konstin
Copy link
Member

@konstin konstin commented Feb 7, 2019

The first commit simplifies the initializers by making them take a value instead of a function. This essentially means removing ||.

The second commit is more opinionated in that it removes py.init[_mut|_ref](...), meaning that you need to call Py::new[_ref|_mut](py, ...). The reason is that this decouples code (the gil token shouldn't be bound to what Py<T> does) and that it makes more apparent that we're actually instantiating a Py<T> here.

I've also discovered that PyRawObject::init never actually returns an error, so I made it infallible (as in it can only fail with a panic/segfault).

Eventually I'd like to make calling in consturctors Py::new unnecessary through into_object, which would also remove the Ok() wrapping introduced in commit two.

@konstin konstin changed the title Remove py.init[_mut|_ref](|| ...) for Py::new[_ref|_mut](py, ...) Change the initializer style Feb 7, 2019
Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -53,7 +53,7 @@ impl MyClass {

#[new]
fn __new__(obj: &PyRawObject, num: i32) -> PyResult<()> {
obj.init(|| {
obj.init( {
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit, but it looks redundant spaces

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed

@kngwyu
Copy link
Member

kngwyu commented Feb 10, 2019

The first commit simplifies the initializers by making them take a value instead of a function. This essentially means removing ||

👍 for this change. Since we don't have PyToken now, there's no reason to use closure here.

The second commit is more opinionated in that it removes py.init[_mut|_ref](...)

I've also discovered that PyRawObject::init never actually returns an error, so I made it infallible

Also 👍

Eventually I'd like to make calling in consturctors Py::new unnecessary through into_object, which would also remove the Ok() wrapping introduced in commit two.

It looks we can allow users to define fn __new__(obj: &PyRawObject) { by just changing proc macro code 🤔
Isn't it enough?

@konstin
Copy link
Member Author

konstin commented Feb 10, 2019

It looks we can allow users to define fn __new__(obj: &PyRawObject) { by just changing proc macro code thinking
Isn't it enough?

Eventually, I'd like to allow writing idiomatic rust style new function, .e.g.:

#[pyo3]
impl Coordinate {
    #[pyo3(constructor)]
    fn new(x: isize, y: isize) -> Self {
        Coordinate { x, y }
    }
}

I know that this is technically possible because I've implemented it in a wrapper, but there's still a bit missing in pyo3 to properly support that.

@konstin konstin force-pushed the init_value branch 3 times, most recently from 8553292 to 1423942 Compare February 12, 2019 22:12
@konstin
Copy link
Member Author

konstin commented Feb 12, 2019

The tests are currently failing, that's why I've cherry picked two working commits into individual pull requests

@konstin konstin force-pushed the init_value branch 2 times, most recently from 6e429ae to bbfefd7 Compare February 13, 2019 11:40
@konstin
Copy link
Member Author

konstin commented Feb 13, 2019

The 3.5 fails due to a travis hiccup, otherwise ci is passing

@konstin konstin merged commit a7b5267 into master Feb 13, 2019
@konstin konstin deleted the init_value branch February 13, 2019 21:29
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.

None yet

2 participants