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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for dunder methods to call their JS equivalent consistently #4037

Closed
Duncanr-glitch opened this issue Aug 4, 2023 · 8 comments
Closed
Labels
enhancement New feature or request

Comments

@Duncanr-glitch
Copy link

馃殌 Feature

If contains or str (or any dunder method, these are just the 2 I ran into personally) are defined in a class, when the includes and toString methods are invoked on instances of that class on the Javascript side, these methods should be run. Currently only the Javascript implementation runs.

However, these functions do work if you call them directly (i.e. cls.contains(value))

Surprisingly, if you get the length property, it does call the len method in the class.

Motivation

Dunder methods are a powerful means of class functionality in python.

Pitch

If a dev has to call method explicitly when there's a JS equivalent to the python operator, then it makes the code less clean and harder to maintain since it makes the code both less pythonic and less natively Javascript-like.

@Duncanr-glitch Duncanr-glitch added the enhancement New feature or request label Aug 4, 2023
@hoodmane
Copy link
Member

hoodmane commented Aug 4, 2023

Can you please give an example of code you wrote, what it currently does, and what you would prefer for it to do?

@Duncanr-glitch
Copy link
Author

Duncanr-glitch commented Aug 4, 2023

I have a python DateRange Class that I want to be able to check other dates membership inside the range. So I made the following dunder method in python:

 def __contains__(self, date_value: Union[datetime, date]) -> bool:
    date_value = self._convert_to_date(date_value)
    return self.lower_date <= date_value <= self.upper_date

but when I run the includes method on an instance of the object in Javascript with a JS Date object as the method's argument, I get a KeyError rather than running my python method.

What I was expecting was that it would work like using the in operator in python and run my __contains__ dunder method. I would expect basically the same with str and the toString method where my str method would run in JS and the same for every other dunder method with an equivalent JS function/method.

@hoodmane
Copy link
Member

hoodmane commented Aug 4, 2023

Well toString() uses __repr__:

let a = pyodide.runPython(`
class A:
   def __repr__(self):
      return "a"
A()
`);
a.toString()

I don't recall why we picked __repr__ over __str__ but there isn't an obvious choice.

For __contains__ we seem to map it to has:

pyodide.runPython(`
class A:
   def __contains__(self, other):
      return isinstance(other, int) and other > 5
A()
`);
a.has(7); // true
a.has(4); // false
a.has({}); // false

If you register it as a Sequence then you can use __contains__ via includes:

let a = pyodide.runPython(`
from collections.abc import Sequence
@Sequence.register
class A:
   def __contains__(self, other):
      return isinstance(other, int) and other > 5
A()
`);
a.includes(7); // true

This is meant to line up with the way JavaScript objects work: there is Array.prototype.includes but Map.prototype.has and Set.prototype.has, etc. So only Sequence-like objects get an includes and everything else gets has instead.

@Duncanr-glitch
Copy link
Author

To the toString method using the __repr__ method, that 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.

I don't know how you're running your pyodide code, but I'm running it through TS-Node and adding @Sequence.register to my code still errors out when using the includes method, as does your example. The has method does work though.

@hoodmane
Copy link
Member

hoodmane commented Aug 4, 2023

Oh yes, the @Sequence.register example is on tip of tree, so it will be supported when we release v0.24.

Looking at the history, it seems that toString was made to do __repr__ when the project was three months old, which was before things were really discussed at all:
9579aa9

Since then I'm not sure if anyone has discussed whether it should do __repr__ or __str__. I would be open to changing it if there is a consensus in favor of __str__.

@Duncanr-glitch
Copy link
Author

Ahh that makes sense for the example. Regardless, it seems that the separation between includes and has in Javascript doesn't apply to python in any sensible way, so I'll leave it at what it is.

I've given my case for __str__, so I'll leave it at that for the maintainers to decide :)

@hoodmane
Copy link
Member

hoodmane commented Aug 4, 2023

I'll open a separate issue and solicit some opinions.

@hoodmane
Copy link
Member

hoodmane commented Aug 4, 2023

Okay I'm going to close this issue @Duncanr-glitch but feel free to reopen it if you think it hasn't been resolved, or open a new issue if you have further questions.

@hoodmane hoodmane closed this as completed Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants