Remove NotImplementedError for parse_dates keyword in read_excel #15820
Conversation
| @@ -1176,13 +1176,18 @@ def _should_parse_dates(self, i): | |||
| if isinstance(self.parse_dates, bool): | |||
| return self.parse_dates | |||
| else: | |||
| name = self.index_names[i] | |||
| if self.index_names is not None: | |||
| name = self.index_names[i] | |||
There was a problem hiding this comment.
@jorisvandenbossche this fixes, though not quite sure why this could/would be None here.
@gfyoung any idea?
There was a problem hiding this comment.
index_names is None by default, though why that wasn't caught before is strange.
There was a problem hiding this comment.
right. yep must not have taken that path at all, e.g. read_csv(..., parse_dates=['column_name'], index_col=0) is all that this is doing.......
if you want to look and see maybe missing something obvious, would be great.
There was a problem hiding this comment.
Sure thing. Just curious, what test was failing beforehand that allowed you to catch this?
There was a problem hiding this comment.
@jorisvandenbossche re-enabled the parse_dates kw in read_excel, but was failing on this. Note that this by-definition is only python engine. (as that is what excel uses).
|
I'll fix the authorship on merge. |
|
Thanks for resurrecting this! Not directly an idea about your question (that was the reason I didn't update the PR, as I didn't found the time to dive into the parsers code to figure this out) |
| "is not implemented") | ||
| if parse_dates is True and index_col is None: | ||
| warn("The 'parse_dates=True' keyword of read_excel was provided" | ||
| " without an 'index_col' keyword value.") | ||
|
|
There was a problem hiding this comment.
Can we add a test that hits this warning?
There was a problem hiding this comment.
added, though I am puzzled why a simple parse_dates=True doesn't just work.....
Rebase and update of PR #12051 Author: Joris Van den Bossche <jorisvandenbossche@gmail.com> Author: Graham R. Jeffries <graham.r.jeffries@gmail.com> This patch had conflicts when merged, resolved by Committer: Jeff Reback <jeff@reback.net> Closes #14326 from jorisvandenbossche/pr/12051 and squashes the following commits: 0b65a7a [Joris Van den Bossche] update wording 656ec44 [Joris Van den Bossche] Fix detection to raise warning b1c7f87 [Joris Van den Bossche] add whatsnew 925ce1b [Joris Van den Bossche] Update tests 0e10a9d [Graham R. Jeffries] remove read_excel kwd NotImplemented error, update documentation #11544
Codecov Report
@@ Coverage Diff @@
## master #15820 +/- ##
==========================================
+ Coverage 90.99% 90.99% +<.01%
==========================================
Files 143 143
Lines 49401 49403 +2
==========================================
+ Hits 44954 44956 +2
Misses 4447 4447
Continue to review full report at Codecov.
|
|
@jorisvandenbossche good with this (it all passed ). ? |
|
Yes, if it passes the tests, good for me! (but as I said, can't really assess if the changes in the parser code are sensible) |
rebase of #12051 and fixes on top
closes #11544