-
Notifications
You must be signed in to change notification settings - Fork 42
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
datastream: optimize memory usage on ORCID sync #439
datastream: optimize memory usage on ORCID sync #439
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice catches overall :)
Only one minor comment on the dict iteration part.
dec7ae4
to
05b5d79
Compare
yield result | ||
} | ||
|
||
for orcid, future in list(futures.items()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is casting to list needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be just iterating of list(futures.keys())
, since the reason is that we're modifying the futures
dictionary as we iterate over the keys. So "consuming" the iterator iterator is necessary. See also #439 (comment)
05b5d79
to
445c2ff
Compare
0e7dae6
to
b60d386
Compare
947daf4
to
375c51d
Compare
375c51d
to
458dc72
Compare
No description provided.