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

Remove ObjectProtocol to make usage more Pythonic #892

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented May 3, 2020

This is a follow up to my comment in #886 . There's two ideas here that I've been thinking about for a while.

I'd be really interested to hear what you think of these ideas, and if you like them, I will finish off this PR according to feedback received.

ObjectProtocol isn't needed any more

  1. I think as we established when talking about get_super() some time ago, it's very nice if PyList etc. implement Deref for PyAny. This is now very easy to do with New Native Types and Lighter GILPool #887 merged.
  2. Once Deref is implemented, I don't think ObjectProtocol serves any special purpose not served by just putting the methods directly on PyAny. So I moved them there.
    • In my opinion this makes documentation better because users just look at the docs for PyAny to see all the methods they can call. Also removing a trait makes less things for users to understand.
    • I think this will make generated code size smaller - all methods are generated only once (for PyAny) rather than for every T which implements ObjectProtocol.

More pythonic repr(), str() etc.

If I have an object o in Python I can get its repr by either doing repr(o) or o.__repr__(). So in my opinion pyo3 should offer the same API.

To allow this, I:

  • Made a module pyo3::builtin_methods which for the moment just has repr, str, len and hash.
  • Added PyAny::__repr__
  • Deprecated PyAny::repr etc.

After this change, I can now call either pyo3::repr(o), or o.__repr__().

TODO:

  • Update documentation
    • Examples for the new PyAny methods
    • Examples for the new pyo3::builtin_methods
  • Perhaps implement similar change for many other methods - getattr, setattr, bool etc. (https://docs.python.org/3/library/functions.html)
  • Complete tests (they were very lacking for ObjectProtocol before)

@kngwyu
Copy link
Member

kngwyu commented May 3, 2020

Thank you, but now I'm thinking about there can be a case we need ObjectProtocol for PyCell.
Let me think about it for a while.

@davidhewitt
Copy link
Member Author

davidhewitt commented May 3, 2020

Thank you, but now I'm thinking about there can be a case we need ObjectProtocol for PyCell.
Let me think about it for a while.

👍 - IMHO PyCell<T> can also support AsRef<PyAny>, and maybe even Deref. Take time to think what you like though.

(I guess these changes also affect rust-numpy, so you may also want to think about how it affects PyArray etc?)

@davidhewitt
Copy link
Member Author

davidhewitt commented May 3, 2020

FWIW I feel more strongly about the names like o.__repr__ and pyo3::repr() than I do about removing ObjectProtocol. So if you don't want to have ObjectProtocol removed, I would propose just changing the names on ObjectProtocol.

@Alexander-N
Copy link
Member

Regarding removing ObjectProtocol, I like the idea of having only one place where to look up all the available methods.

About having repr() etc. as free functions as well as methods, I would prefer not having multiple ways to do the same thing and I don't think it would make PyO3 more pythonic. In Python a user should always use repr() while __repr__() is more like the interface you would implement, but which would not be called directly, so I don't see having both as the public API which should get replicated. Instead of having a pythonic PyO3, I would rather like to have PyO3 feel natural from the Rust side and would greatly prefer calling for example .len() instead of pyo3::len().

@davidhewitt
Copy link
Member Author

davidhewitt commented May 3, 2020

That's a reasonable argument @Alexander-N 👍

Maybe here's another idea - what do you think if we supported both repr(o) and o.repr()? (And we forget about my proposal o.__repr__().)

Then those like me who want to use these functions like in Python can write repr(o) (and can easily write use pyo3::builtin_methods::*;)

And also those like yourself who prefer the Rustic way can use o.repr().

I mostly agree with you there's an argument that "there should be only one obvious way" - but I think on this point the obvious way maybe differs if you're a Python user expecting to do repr(o), or a Rust user looking for a method o.repr().

@Alexander-N
Copy link
Member

I think as soon as several people work on one codebase there will be a cost in enabling inconsistencies which for me outweighs the benefit of having different APIs for different preferences. Also, I fear that this will cause confusion to new users, as to what's the difference between len() and .len and when to use which (this is confusing a lot of Python beginners). In Python there is a good reason for having both, which I'm not seeing in our case. But I totally understand that all this is very subjective.

@birkenfeld
Copy link
Member

Regarding removing ObjectProtocol, I like the idea of having only one place where to look up all the available methods.

I think the idea here is that ObjectProtocol is just redundant since it would only be implemented for PyAny. So there's still only one place to look them up.

@m-ou-se
Copy link
Contributor

m-ou-se commented May 4, 2020

Note that x.__bla__() and bla(x) isn't always the exact same. For example:

>>> dir(None)
['__bool__', '__class__', '__delattr__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__']
>>> None.__dir__()
['__repr__', '__bool__', '__new__', '__doc__', '__hash__', '__str__', '__getattribute__', '__setattr__', '__delattr__', '__lt__', '__le__', '__eq__', '__ne__', '__gt__', '__ge__', '__init__', '__reduce_ex__', '__reduce__', '__subclasshook__', '__init_subclass__', '__format__', '__sizeof__', '__dir__', '__class__']

@birkenfeld
Copy link
Member

Since it is quite hard to see at a glance: the difference is that dir() sorts the list returned by __dir__.

@davidhewitt
Copy link
Member Author

Strange! I wonder if this is a CPython implementation detail, or is specified in docs. I'll have a dig 😄

@kngwyu
Copy link
Member

kngwyu commented May 5, 2020

I'll have a dig

Please wait for a bit...

  1. it's very nice if PyList etc. implement Deref for PyAny

OK. No problem except it can look odd to use downcast for &PyList or so.

  1. I don't think ObjectProtocol serves any special purpose not served by just putting the methods directly on PyAny

The only concern I had was it can be separated into ObjectProtocol and ObjectProtocolMut, which fit nicely PyRef and PyRefMut. But since such an implementation is quite difficult, I gave up the idea. For now, let's implement Deref<&PyAny> only for &PyCell(The same as the current situation. Actually ObjectProtocol is implemented for &PyCell now). Then we don't have to think about the borrow problem.

  1. More pythonic repr(), str() etc.

As a user, I don't want them. And as a maintainer, I haven't heard that any user complains about that(e.g., we have no issue like 'why PyO3 doesn't have repr or str?').
I'm kind of doubtful we need them, especially if we need some complex implementations to provide compatible dir or so.

@birkenfeld
Copy link
Member

1. > More pythonic repr(), str() etc.

As a user, I don't want them. And as a maintainer, I haven't heard that any user complains about that(e.g., we have no issue like 'why PyO3 doesn't have repr or str?').
I'm kind of doubtful we need them, especially if we need some complex implementations to provide compatible dir or so.

FWIW, I agree. I'm perfectly happy with methods, as long as the names and their correspondence are clear (e.g. .repr()).

If we want to introduce APIs that mimic the behavior of more complex built-in Python functions, they should probably be free functions as well.

@davidhewitt
Copy link
Member Author

davidhewitt commented May 5, 2020

Ok I will give up on the builtin_methods for now, and make this PR purely about moving ObjectProtocol to PyAny. Thanks for the feedback all 👍

@davidhewitt davidhewitt closed this May 6, 2020
@davidhewitt davidhewitt deleted the simpler-objectprotocol branch May 6, 2020 18:43
@davidhewitt
Copy link
Member Author

I'll follow up with the simpler changes as a separate PR, for easier reviewing.

@davidhewitt davidhewitt mentioned this pull request May 7, 2020
macisamuele added a commit to macisamuele/json-trait-rs that referenced this pull request May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants