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 RecursionError when iterating streams #1554

Merged
merged 2 commits into from
Feb 12, 2023

Conversation

eric-wieser
Copy link
Contributor

next(self) was previously just a recursive call that does nothing.

This now implements __next__ using the actual implementation, and lets the python2-compatibility next() just wrap it.

@Byron
Copy link
Member

Byron commented Feb 12, 2023

Thanks a lot, I have a feeling that this issue was present for a long time already!

Do you think Python 2 compatibility is still required? Support for it was dropped a while ago and if you agree it can be dropped entirely.

@eric-wieser
Copy link
Contributor Author

I have no objection to dropping the next method entirely; I left it only for the sake of being conservative.

Comment on lines +704 to +705
next = __next__

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to commit through the github UI if you want to drop this Python2 compatibility layer entirely:

Suggested change
next = __next__

Note that there may be some downstream code that relies on using next because __next__ was broken though!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Oh well, let's be conservative then.

@Byron
Copy link
Member

Byron commented Feb 12, 2023

Great! Let's do that and the PR can be merged. Thank you

@Byron Byron added this to the v3.1.31 - Bugfixes milestone Feb 12, 2023
@Byron Byron merged commit 202e31c into gitpython-developers:main Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants