-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
REF: Split up parsers into smaller files #39217
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
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.
i would update pandas/io/parsers/__init__.py
to import read_csv, read_fwf, read_table
, and other public functions here (if any others).
you can leave .readers
just import them to __init__.py
(then you don't need to change the other modules that import).
otherwise this is a straight move?
Thanks, will do that. Yes, just moved them around. Will go to refactoring after this |
I moved the relevant functions up, I changed the import paths for the helper functions in the test functions |
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 ping on green
https://github.com/pandas-dev/pandas/pull/39217/checks?check_run_id=1730796224 looks like a real fail |
Have to check, this one worked 3 days ago in the pipelines |
Hm merged master, let's check if this persists. Can not reproduce this locally |
Additionally this worked the commit before when I forgot a few imports. At least a place to start pinning if it persists |
Green now @twoertwein I think you worked with unclosed handles the last few days/weeks? Could you maybe have a look and see if this is familiar? |
Failing https://github.com/pandas-dev/pandas/runs/1731152527?check_suite_focus=true too, unrelated here then |
I assume that most of the ssl warnings come from boto3 (I honestly gave up on them for now in #39047). Wasn't a test like that recently xfailed (reason s3/boto3 being updated)? |
Yes, but this test really failed not only raised a warning, |
do you mean the or |
Sorry this was not clear enough by myself. I was talking about |
return TextFileReader(*args, **kwds) | ||
|
||
|
||
def _clean_na_values(na_values, keep_default_na=True): |
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.
might be worth moving from here down to another module (validation.py?) in a followup, e.g. keep readers.py for the actual readers
thanks @phofl |
Refactored them into 4 files, similar to described in the issue. If this is accepted I will try to clean this up a bit more through unifying the calls a bit