REF/CLN: pandas/io/parsers.py #36852
Conversation
ivanovmg
requested review from
jorisvandenbossche,
TomAugspurger and
jbrockmendel
|
big picture: whats the goal? |
Extract some methods to make any further refactoring easier. |
pandas/io/parsers.py
Outdated
| if result.get(arg, depr_default) != depr_default: | ||
| depr_warning += msg + "\n\n" | ||
| msg = ( | ||
| f"The {repr(arg)} argument has been deprecated and will be " |
Is there further refactoring you have in mind? |
pandas/io/parsers.py
Outdated
| @@ -951,6 +903,61 @@ def __init__(self, f, engine=None, **kwds): | |||
| def close(self): | |||
| self._engine.close() | |||
|
|
|||
| @staticmethod | |||
pandas/io/parsers.py
Outdated
| kwds[param] = dialect_val | ||
| return kwds | ||
|
|
||
| @staticmethod |
pandas/io/parsers.py
Outdated
| return kwds | ||
|
|
||
| @staticmethod | ||
| def _validate_skipfooter(kwds): |
pandas/io/parsers.py
Outdated
| @@ -951,6 +903,61 @@ def __init__(self, f, engine=None, **kwds): | |||
| def close(self): | |||
| self._engine.close() | |||
|
|
|||
| @staticmethod | |||
| def _refresh_kwargs_based_on_dialect(kwds, dialect): | |||
There was a problem hiding this comment.
can you type and add a doc-string
pandas/io/parsers.py
Outdated
| self._engine_specified = kwds.get("engine_specified", engine_specified) | ||
|
|
||
| if kwds.get("dialect") is not None: | ||
| dialect = kwds["dialect"] | ||
| if dialect in csv.list_dialects(): | ||
| dialect = csv.get_dialect(dialect) | ||
| kwds = self._refresh_kwargs_based_on_dialect(kwds, dialect) |
There was a problem hiding this comment.
merge master as some of this updated; and rename this like check_defaults_dialect (or similar)
There was a problem hiding this comment.
Done.
I renamed the function to _merge_with_dialect_properties as it actually merges kwargs with those provided by dialect (and resolve some conflicts).
I am still trying to figure out what the code does. By making small PRs on refactoring/cleanup I get to understand it better. Probably later I will have some suggestion on the refactoring. |
| @@ -3791,3 +3735,93 @@ def _check_defaults_read( | |||
| kwds["engine_specified"] = False | |||
|
|
|||
| return kwds | |||
|
|
|||
|
|
|||
| def _merge_with_dialect_properties( | |||
There was a problem hiding this comment.
reverse these (always put kwds things last).
There was a problem hiding this comment.
rename kwds -> defaults
so similar to the _check_defaults_read
also not averse to renaming _check_defaults_read to something better.
There was a problem hiding this comment.
I have done it.
Also, I found the issue with the ValueError message, when dialect is incorrect.
Originally, kwds['dialect'], but I decided to use dialect.__name__ for better readability, which is not accepted by mypy (throws error "Dialect" has no attribute "__name__" [attr-defined]).
So, now I use string representation of dialect instance, however I can still pass one extra parameter (dialect_name) to _merge_with_dialect_properties and use it in the error message.
|
cc @twoertwein if any comments. |
|
LGTM, outsourcing the dialect validation makes the constructor of |
|
Thanks @ivanovmg! |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffRefactor/cleanup
pandas/io/parsers.py