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

Question: Should toString use __repr__ or __str__? #4040

Closed
hoodmane opened this issue Aug 4, 2023 · 5 comments · Fixed by #4247
Closed

Question: Should toString use __repr__ or __str__? #4040

hoodmane opened this issue Aug 4, 2023 · 5 comments · Fixed by #4247
Labels

Comments

@hoodmane
Copy link
Member

hoodmane commented Aug 4, 2023

In #4037 (comment) @Duncanr-glitch said:

the toString method using the __repr__ method just seems incorrect since there's a distinction between __repr__ and __str__ that python devs expect and sometimes rely on. Using __repr__ when doing a bare console.log makes since, but not when a dev is trying to string the object.

Looking at the history, toString was added here: 9579aa9
I am not aware that we've ever discussed what this should do. I personally don't have a strong opinion. Interestingly, toString is missing from our proxy table, we should probably update the table in any case.
https://pyodide.org/en/stable/usage/type-conversions.html#proxying-from-python-into-javascript

@rth @ryanking13 @antocuni what do you all think?

@hoodmane hoodmane added the enhancement New feature or request label Aug 4, 2023
@antocuni
Copy link
Contributor

antocuni commented Aug 4, 2023

Same as you, I don't have a strong opinion but I have a very mild preference for __str__: JS doesn't even try to do anything useful with toString() anyway, ([object Object] anyone?), so both python methods are equally reasonable, but __str__ has a more similar name :)

@hoodmane hoodmane added Question and removed enhancement New feature or request labels Aug 4, 2023
@rth
Copy link
Member

rth commented Aug 5, 2023

I have a very mild preference for str

Same for me. +0.5 for using __str__.

For repr, I find it weird that if you take for instance x: str = "1" in Python and call repr on it you would get "'1'", which then is different from the behavior in JS. At least for strings, if we were proxy rather than convert, __str__ matches the current behavior. So maybe would then make sense to do it for proxies as well.

@ryanking13
Copy link
Member

I'm also +1 for__str__. To be more precise, +1 for str (PyObject_Str) than repr (PyObject_Repr), as str will fallback to __repr__ if __str__ does not exist.

@hoodmane
Copy link
Member Author

hoodmane commented Aug 6, 2023

Okay, since all are agreed that using __str__ is preferable, I will open a PR. Does anyone object to changing this in v0.24.0? Maybe we should add a flag to opt into the old behavior with a deprecation period?

@ryanking13
Copy link
Member

Maybe we should add a flag to opt into the old behavior with a deprecation period?

This sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants