-
Notifications
You must be signed in to change notification settings - Fork 262
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
bump pyo3 to 0.17.1 #236
bump pyo3 to 0.17.1 #236
Conversation
Will do, currently fighting with v1.10. |
cannot infer type of the type parameter `V` declared on the associated function `set_item`
the trait bound `&pyo3::PyAny: pyo3::IntoPy<pyo3::Py<PyString>>` is not satisfied
9988f63
to
e3e4f3f
Compare
Codecov Report
Continue to review full report at Codecov.
|
@samuelcolvin @davidhewitt This is ready for review but I feel like this is hacky. If you have a better way of doing it I'm all ears 🙏 |
src/input/datetime.rs
Outdated
Some(offset) => { | ||
let tz_info = TzInfo::new(offset); | ||
Some(Py::new(py, tz_info)?.to_object(py)) | ||
let obj = Py::new(py, tz_info)?.to_object(py); |
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 think the following might work for these three lines:
let obj = Py::new(py, tz_info)?.to_object(py); | |
Some(Py::new(py, tz_info)?.to_object(py).extract(py)?) |
Will rely on type inference for .extract()
to give you the &PyTzInfo
.
I do think there's some missing API and documentation for PyO3 in this area, just haven't quite decided what the API should look like.
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.
Thank you! I first tried that but had issue with lifetime of variable since Py::new
takes ownership of the pointer (hence my usage of from_borrowed_ptr
instead. I fixed it in 8f09d8c
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.
LGTM, @PrettyWood merge if you think this is done.
fix #214