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

Fix return type of Timeline.__iter__ #94

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

domsmrz
Copy link
Contributor

@domsmrz domsmrz commented Jan 29, 2024

Small fix of provided typing. Timeline.__iter__ already returns iterator (not just iterable) as required -- This change propagates this information to the actual annotation.

@hbredin
Copy link
Member

hbredin commented Jan 29, 2024

This is some code I wrote some long time ago and I am not quite sure about the difference between Iterable and Iterator.

Also, would it make sense to stick to Iterable and remove the call to iter?

@domsmrz
Copy link
Contributor Author

domsmrz commented Jan 29, 2024

I believe the proposed change is good as-is:

  • Iterator is required to have __next__, so it can be passed into next method (as in next(my_iterator)). It can be somewhat thought of as a pointer to a specific place inside a list or another container (and the next method is a way to make the pointer move to the next element).
  • Iterable on the other hand is required to have __iter__ method. The __iter__ method should return Iterator (via which one can iterate over the Iterable). Classical example of Iterables are lists.

Additionally, IIRC Iterators should also implement __iter__ method, meaning every Iterator is also Iterable, but that is not really important here.

Hence, the iter call at the end of the method is required (otherwise the method would return Iterable and not Iterator but __iter__ has to return Iterator, otherwise -- for example -- for loops wouldn't work).

@hbredin hbredin merged commit de24bc7 into pyannote:develop Feb 12, 2024
4 checks passed
@hbredin
Copy link
Member

hbredin commented Feb 12, 2024

Thanks!

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