Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 24, 2025

Not sure this is a good idea or not.

Motivation: we have too many fragmented traits, which are easy to forget to add in with() attrs.

Now __str__ will more looking like a normal #[pymethod]. But what it backs will be very different. It actually doesn't create a real pymethod, but actually creating slot_str. And __str__ will be placed by a slot-wrapper descriptor.
After the patch, #[pymethod] fn __str__ will additionally create a pyslot for str.

This can be applicable to other single-method traits too.

Pros: Easy to understand. No worry to forget to add with()
Cons: Confusing because it have different semantics comparing to other real pymethods.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 24, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone force-pushed the __str__ branch 4 times, most recently from 05933fe to d4a2177 Compare December 24, 2025 14:36
@ShaharNaveh
Copy link
Collaborator

Not sure this is a good idea or not.

Motivation: we have too many fragmented traits, which are easy to forget to add in with() attrs.

Now __str__ will more looking like a normal #[pymethod]. But what it backs will be very different. It actually doesn't create a real pymethod, but actually creating slot_str. And __str__ will be placed by a slot-wrapper descriptor. After the patch, #[pymethod] fn __str__ will additionally create a pyslot for str.

This can be applicable to other single-method traits too.

Pros: Easy to understand. No worry to forget to add with() Cons: Confusing because it have different semantics comparing to other real pymethods.

Here are my concerns:

  1. Will this lead to implementation difference with CPython where an entire CPython test suite will not be applicable? (so we will have to maintain our own)
  2. I ain't so sure why __str__ was chosen for this PR while other dunder methods like __repr__ were not for example. This concern can be addressed with a detailed comment about this change

@youknowone
Copy link
Member Author

  1. Will this lead to implementation difference with CPython where an entire CPython test suite will not be applicable? (so we will have to maintain our own)

No, this is a sort of developer interface. CPython test suites test the implementation. It doesn't matter if we use a trait or fake pymethod.

  1. I ain't so sure why __str__ was chosen for this PR while other dunder methods like __repr__ were not for example. This concern can be addressed with a detailed comment about this change

Because this form is an alternative form of the trait approach, which is already implmemented for __repr__. If we use same approach for repr, then trait Representable will be replaced by #[pymethod] fn __repr__. So if we go this way, __repr__ will look like __str__ without trait Representable

Copy link
Collaborator

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

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

All concerns addressed at #6485 (comment)

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.

2 participants