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 the TokenTable iterator implementation #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

troygraben
Copy link

The __iter__ function must perform initialization and return the iterator
object itself. The __next__ function must return the next item or raise
StopIteration when no more elements are available.

This fixes the StopIteration exception being uncaught by the for loop when
the iterator reaches the end of the collection. This could be encountered
when calling smbios-token-ctl --dump-tokens.

@superm1
Copy link
Contributor

superm1 commented Jul 26, 2021

Cc @srinivasgowda and @GoPerry

@Celelibi
Copy link

This is actually the cause of issue #86 (which has been closed for some reason).
The issue arise because of PEP 0479. Since python 3.7 raising a StopIteration exception explicitely inside a generator is deprecated and now raises a RuntimeException.

I think a much simpler fix for this could be to turn the raise StopIteration into a return. In its current state, this PR would disallow concurrent iterations of the TokenTable. (Moreover, calling __next__ on a iterator that already eached the end would call still token_table_get_next, which seems pretty surprising.)

This change supports PEP 479 by changing the generator to use return
instead of raising StopIteration.

This fixes the RuntimeError exception being raised with Python 3.7 and
newer when the iterator reaches the end of the collection. This could be
encountered when calling smbios-token-ctl --dump-tokens
@troygraben
Copy link
Author

@Celelibi
Your approach sounds much more sensible to me. It seems that many docs online still reference using StopIteration even though this PEP 0479 is has been in place for many years.

@superm1
Copy link
Contributor

superm1 commented Jul 31, 2021

Cc @dell-client-linux

@kalvdans
Copy link

kalvdans commented May 5, 2023

This change fixes the problem for me as well. Please merge :)

@4oijq342
Copy link

4oijq342 commented Sep 3, 2023

Hi, still experiencing this issue.

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.

5 participants