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

Add support for the reversed() built-in on appropriate iterators #319

Closed
rpdelaney opened this issue Nov 19, 2018 · 5 comments
Closed

Add support for the reversed() built-in on appropriate iterators #319

rpdelaney opened this issue Nov 19, 2018 · 5 comments

Comments

@rpdelaney
Copy link

rpdelaney commented Nov 19, 2018

https://docs.python.org/3/library/functions.html#reversed

reversed(seq)

    Return a reverse iterator. seq must be an object which has 
a __reversed__() method or supports the sequence protocol 
(the __len__() method and the __getitem__() method with integer 
arguments starting at 0).

As discussed offline, I'm planning on implementing support for this built-in with a PR when I can get the time.

Discussion in #95 might be relevant.

@niklasf
Copy link
Owner

niklasf commented Nov 30, 2018

Thanks for your PR. I implemented it a little bit differently:

  • Not using node[n] for now.
  • node.mainline() is an iterator over the nodes starting at (and excluding) node.
  • reversed(node.mainline()) is the corresponding reversed iterator.
  • node.mainline().accept(visitor) works.

__getitem__, __len__ and maybe others could be added on node.mainline(). I didn't do this to start with, because it appears to be no less efficient than working with list(node.mainline()).

Note that master breaks backwards compability with 0.23.x.

@rpdelaney
Copy link
Author

Cool, I'll test this out.

As for compatibility, it looks like that's broken due to doing s/main_line/mainline in various places. Is that all?

@niklasf
Copy link
Owner

niklasf commented Nov 30, 2018

s/main_line/mainline_moves/, some more renamed methods, node.accept(...) includes the move leading to node.

@rpdelaney
Copy link
Author

Just curious, but is that rename really worth breaking compatibility? You could maintain compatibility by leaving the old form behind pointing to the new form, but ... idk, seems like a lot of extra work just to kill an innocent underscore. :)

@niklasf
Copy link
Owner

niklasf commented Nov 30, 2018

It does a different thing now: It iterates over nodes, where it previously was iterating over moves. So might as well change the name, which I always wanted to do anyway.

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

No branches or pull requests

2 participants