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: show pattern for self.__class__.__name__ in __repr__ #3067

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

wjones127
Copy link
Contributor

@wjones127 wjones127 commented Mar 26, 2023

It took me a little while to figure out this pattern in PyO3, so I thought it would be a good addition to the guide.

It's not the cleanest pattern, so would welcome suggestions on how to make it shorter or easier.

@messense messense added the CI-skip-changelog Skip checking changelog entry label Mar 27, 2023
guide/src/class/object.md Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

Thank you for sharing your findings! I agree that it is not obvious how to approach this. However, I would suggest not burdening this particular example with the complication but to start a new example on how to access the class name.

Concerning the implementation, instead of starting with PyRef, I think it would simpler to use PyCell which is the actual shape of the Python object and therefore directly derefs into PyAny creating another strong reference, e.g.

diff --git a/guide/src/class/object.md b/guide/src/class/object.md
index 357fdcd7..5b099ed3 100644
--- a/guide/src/class/object.md
+++ b/guide/src/class/object.md
@@ -53,13 +53,17 @@ contained inside `Number`.
 impl Number {
     // For `__repr__` we want to return a string that Python code could use to recreate
     // the `Number`, like `Number(5)` for example.
-    fn __repr__(&self) -> String {
+    fn __repr__(self_: &PyCell<Self>, py: Python<'_>) -> PyResult<String> {
+        let class_name: String = self_
+            .getattr("__class__")?
+            .getattr("__name__")?
+            .extract()?;
         // We use the `format!` macro to create a string. Its first argument is a
         // format string, followed by any number of parameters which replace the
         // `{}`'s in the format string.
         //
         //                       👇 Tuple field access in Rust uses a dot
-        format!("Number({})", self.0)
+        Ok(format!("{class_name}({})", self_.borrow().0))
     }
 
     // `__str__` is generally used to create an "informal" representation, so we

However, for this particular issues (getting the name of Rust-implemented Python class), there is simpler way IIUC via our own PyTypeInfo trait, e.g.

diff --git a/guide/src/class/object.md b/guide/src/class/object.md
index 357fdcd7..c2d0865f 100644
--- a/guide/src/class/object.md
+++ b/guide/src/class/object.md
@@ -44,7 +44,7 @@ It can't even print an user-readable representation of itself! We can fix that b
 contained inside `Number`.
 
 ```rust
-# use pyo3::prelude::*;
+# use pyo3::{prelude::*, PyTypeInfo};
 #
 # #[pyclass]
 # struct Number(i32);
@@ -59,7 +59,7 @@ impl Number {
         // `{}`'s in the format string.
         //
         //                       👇 Tuple field access in Rust uses a dot
-        format!("Number({})", self.0)
+        format!("{}({})", Self::NAME, self.0)
     }
 
     // `__str__` is generally used to create an "informal" representation, so we

which I would probably prefer to show case. But I am not sure whether we want to guide user's towards these "infrastructure traits" too strongly?

@birkenfeld
Copy link
Member

The second one is also incorrect in subclasses.

But couldn't the lookup of class and name be done faster with native APIs? self.get_type().name()?

@adamreichold
Copy link
Member

But couldn't the lookup of class and name be done faster with native APIs? self.get_type().name()?

Indeed! But you'd still have to go via PyCell<Self> to access PyAny::get_type, e.g. self_.get_type().name(), right?

@davidhewitt
Copy link
Member

davidhewitt commented Mar 28, 2023

But I am not sure whether we want to guide user's towards these "infrastructure traits" too strongly?

I would be in favour of not doing so, I have been pondering a move of PyTypeInfo::NAME and PyTypeInfo::MODULE to PyClassImpl, so I'd prefer not encourage users to access those members :)

(While NAME is probably reasonable to have there, I don't see a clear need for it given runtime access is more reliable. MODULE is likely unhelpful because it's not even required to be populated, and types are often accessed via a public API module which isn't where they're actually defined.)

@adamreichold
Copy link
Member

adamreichold commented Mar 28, 2023

I would be in favour of not doing so, I have been pondering a move of PyTypeInfo::NAME and PyTypeInfo::MODULE to PyClassImpl, so I'd prefer not encourage users to access those members :)

Ok, so I think the consensus so far is:

  • Use a separate example, specifically on the topic "access the Python-level class object of Rust-defined #[pyclass]".
  • Use &PyCell<Self> as the receiver as it directly derefs to PyAny.
  • Use the native methods like PyAny::get_type instead of access attributes by name.

@wjones127
Copy link
Contributor Author

wjones127 commented Mar 29, 2023

Thanks for the advice. Btw #1059 would still be really useful; I had no idea you could take &PyCell<Self> as a parameter. If I understood this better right now I would write it myself.

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

LGTM, let's give it a while for others to have a look and if there are no objections we can merge. Thanks again for taking the time to improve the docs!

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

LGTM, just a small nit

guide/src/class/numeric.md Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

@wjones127 Could you squash your commits before merging? Thanks.

@adamreichold
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Mar 30, 2023
3067: docs: show pattern for self.__class__.__name__ in __repr__ r=adamreichold a=wjones127

It took me a little while to figure out this pattern in PyO3, so I thought it would be a good addition to the guide.

It's not the cleanest pattern, so would welcome suggestions on how to make it shorter or easier.

Co-authored-by: Will Jones <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 30, 2023

Build failed:

@adamreichold
Copy link
Member

I force-pushed to remove the usage of inline format arguments.

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 30, 2023

Build succeeded:

@bors bors bot merged commit 77fc6e6 into PyO3:main Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants