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

docs: update Python classes section of the guide #3914

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Feb 28, 2024

Part of #3684

This adapts the "Python classes" section of the guide to the new Bound API.

@@ -1303,7 +1304,7 @@ impl pyo3::impl_::pyclass::PyClassImpl for MyClass {
static DOC: pyo3::sync::GILOnceCell<::std::borrow::Cow<'static, ::std::ffi::CStr>> = pyo3::sync::GILOnceCell::new();
DOC.get_or_try_init(py, || {
let collector = PyClassImplCollector::<Self>::new();
build_pyclass_doc(<MyClass as pyo3::PyTypeInfo>::NAME, "", None.or_else(|| collector.new_text_signature()))
build_pyclass_doc(<MyClass as pyo3::PyTypeInfo>::NAME, "\0", collector.new_text_signature())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just expanded the #[pyclass] macro using rust-analyzer and updated whats changed. 😇

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks very much for kicking off the docs migration!

I think we should probably give existing users of PyCell a nudge to use Bound. Probably worth adding to the migration section as part of this PR?

I wonder if we should also deprecate PyCell completely, to give a stronger push? I suggested it before (I think it's an item on the tracking issue), and I think it'll make sense to do so given where we're heading for the API.

let dict: &PyDict = unsafe { py.from_borrowed_ptr_or_err(self_.as_ptr())? };
fn set(slf: &Bound<'_, Self>, key: String, value: &PyAny) -> PyResult<()> {
slf.borrow_mut().counter.entry(key.clone()).or_insert(0);
let dict = slf.downcast::<PyDict>()?;
Copy link
Member

Choose a reason for hiding this comment

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

👍 yep I think even though downcast_unchecked would be ok here I think best to recommend the safe option in the guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the intention. The code from the guide is often copied as a first step, so unsafe should be avoided I think.

guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
@@ -627,6 +627,12 @@ impl IntoPy<Py<PyTuple>> for Bound<'_, PyTuple> {
}
}

impl IntoPy<Py<PyTuple>> for &'_ Bound<'_, PyTuple> {
Copy link
Member

Choose a reason for hiding this comment

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

Yep this makes sense to add I think, for simplicity of using as argument tuple.

@Icxolu
Copy link
Contributor Author

Icxolu commented Feb 29, 2024

I wonder if we should also deprecate PyCell completely, to give a stronger push? I suggested it before (I think it's an item on the tracking issue), and I think it'll make sense to do so given where we're heading for the API.

I think that would make sense. I have worded the entry in the migration guide accordingly and prepared a commit doing the deprecation. I will push that as a followup PR since the diff is also decently large.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks this looks great! Will read the followup PR next...

@davidhewitt davidhewitt added this pull request to the merge queue Mar 1, 2024
Merged via the queue into PyO3:main with commit 1d22461 Mar 1, 2024
42 checks passed
@Icxolu Icxolu deleted the doc-guide-class branch March 1, 2024 18:35
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.

3 participants