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

Render typed iterators (e.g. Iterator[int]) in docstrings #2244

Closed
wants to merge 1 commit into from

Conversation

sizmailov
Copy link
Contributor

@sizmailov sizmailov commented Jun 5, 2020

This PR makes pybind11 generate much more informative typed iterators.

this PR
__iter__(self: example.VectorInt) -> Iterator[int]
__iter__(self: example.MapTupleToComplex) -> Iterator[Tuple[int, str]]
items(self: example.MapTupleToComplex) -> Iterator[Tuple[Tuple[int, str], complex]]
master
__iter__(self: example.VectorInt) -> iterator
__iter__(self: example.MapTupleToComplex) -> iterator
items(self: example.MapTupleToComplex) -> iterator

Strictly speaking its a potential breaking change but seems like no existing code is affected, see below.

This PR makes py::make_iterator and py::make_key_iterator return py::detail::iterator_state instance instead of py::iterator.
It doesn't affect regular use of those functions (immediate return from __iter__ lambda),
but requires changes in user code with implicit assignments/conversions, e.g.:

// next line is ok now, but will break with this PR
// fix on user side: apply py::cast() explicitly
py::iterator it = make_iterator(...); 

Search for code that would break:
To justify intuition about low frequency of "not-immediate-return" make_iterator usage I've turned to github search API for py::make_iterator. Search is limited to 1000 files, so can't tell only for first 1000 found files (~5726 in total [web search]).

In 1000 files there are 229 unique strings with return py:make_iterator.
The only non-comment occurrence of py::make_iterator without preceding return was found in (pybind?) test code here. This should also be no problem since cast should happen automatically.

py::make_map_iterator search find only two unique strings with preceding return in another 1000 files (did a separate search). No other usages. Looks like this function is much less popular.

While strictly speaking a breaking change was introduced it seems like it breaks no existing code.
If you think this argument is not applicable and change to py::make_iterator should go to major pybind release (e.g. 3.0).

compare.cpp
#include <map>
#include <complex>

#include "pybind11/pybind11.h"
#include "pybind11/stl_bind.h"
#include "pybind11/embed.h" 

namespace py = pybind11;

using map_tuple_to_complex = std::map<std::tuple<int,std::string>, std::complex<double>>;
PYBIND11_MAKE_OPAQUE(std::vector<int>);
PYBIND11_MAKE_OPAQUE(map_tuple_to_complex);

PYBIND11_EMBEDDED_MODULE(example, m) {
  py::bind_vector<std::vector<int>>(m, "VectorInt");
  py::bind_map<map_tuple_to_complex>(m, "MapTupleToComplex");
}

int main() {
  py::scoped_interpreter guard{};
  py::exec(R"(
      from example import *
      print(VectorInt.__iter__.__doc__, end="")
      print(MapTupleToComplex.__iter__.__doc__, end="")
      print(MapTupleToComplex.items.__doc__, end="")
)");
}

This commit introduces minor breaking change: `make_iterator` and `make_key_iterator`
now return `iterator_state` instance instead of `py::iterator`.
It doesn't affect regular use of those functions (immediate return from __iter__ lambda),
but requires changes in user code with implicit assignments/conversions, e.g.:

py::iterator it = make_iterator(...); # requires explicit py::cast()
@sizmailov sizmailov force-pushed the doc-gen-typed-iterators branch from 1701f0c to 9531c6f Compare June 8, 2020 17:20
@wjakob
Copy link
Member

wjakob commented Jun 10, 2020

I'm against merging this -- first, because of the incompatibility, and because it adds a bunch of code for what is a relative niche feature. This is generally a dangerous rabbit hole: we could increase the complexity of pybind11 to generate near-perfect type annotations, but I don't think that is in the interest of the majority of the users of this project. My goal post is: do an okay job in a minimal fashion.

@wjakob wjakob closed this Jun 10, 2020
@wjakob
Copy link
Member

wjakob commented Jun 10, 2020

(Side note: sorry for rejecting it after you've clearly put quite a bit of work into the PR and explanations above).

@sizmailov
Copy link
Contributor Author

That's sad. Would it make a difference if this PR won't be breaking?
I can add _ng versions of py::make*_iterator and use them in std bindings. py::make*_iterator will be marked as deprecated.

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