-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
CLN: use stdlib Iterator instead of BaseIterator #30370
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.
Nice this makes a lot of sense!
pandas/io/common.py
Outdated
@@ -595,6 +584,8 @@ def readline(self) -> bytes: | |||
def next(self) -> bytes: |
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.
Maybe just rename this function?
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.
yah i think that should work
@@ -2,6 +2,7 @@ | |||
|
|||
import bz2 | |||
import codecs | |||
from collections.abc import Iterator |
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.
this should be failing ci on Check for non-standard imports
log shows
##[error]pandas/io/stata.py:12:from collections.abc import Iterator
##[error]pandas/io/sas/sas7bdat.py:16:from collections.abc import Iterator
##[error]pandas/io/sas/sas_xport.py:10:from collections.abc import Iterator
##[error]pandas/io/common.py:5:from collections.abc import Iterator
##[error]pandas/io/parsers.py:6:from collections.abc import Iterator
##[error]pandas/io/json/_json.py:2:from collections.abc import Iterator
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.
changes to invgrep in #29915. maybe related. @datapythonista
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.
It's strange, because I tested that the exit codes were correct in that PR, but I agree the problem is likely to be introduced in that PR.
I'm travelling without my computer at the moment, and I can't test myself. But feel free to revert that PR if you identify it's the one causing the problem, and I'll redo it with the problem fixed later.
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.
what is the preferred way to do the import?
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.
What is the correct way to do the import? I'm getting this failure in my PR https://github.com/pandas-dev/pandas/pull/30151/checks?check_run_id=362567629
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.
I think the import is correct. Must be something wrong with the invgrep as noted by @simonjayhawkins
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.
@alimcmaster1 might have an idea here as well
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.
Sure from reading #25957 (review) where this check was implemented our preferred way to do the import is from collections import abc
. Or from collections import Iterator
I can update this now if helpful @mroeschke ?
Agree - seems to be something flakey with the invgrep on github actions since fails for me locally.
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.
That'd be great @alimcmaster1, thanks!
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.
@jbrockmendel lgtm apart from code-checks
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, x @simonjayhawkins comment
needs a rebase as well |
rebased+green, not sure if the code-checks thing needs action here |
Thanks @jbrockmendel |
No description provided.