-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
BUG: error in read_excel with some ods files #45598 #46050
Conversation
pandas/io/excel/_odfreader.py
Outdated
sheet_cells = [ | ||
x | ||
for x in sheet_row.childNodes | ||
if "qname" in dir(x) and x.qname in cell_names |
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.
Can we do getattr(x, "qname")?
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.
@phofl The tests are passing only with if "qname" in dir(x) and getattr(x, "qname") in cell_names
. But I don't know if that's what you meant.
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.
use hasattr
not dir
pandas/io/excel/_odfreader.py
Outdated
sheet_cells = [ | ||
x | ||
for x in sheet_row.childNodes | ||
if "qname" in dir(x) and x.qname in cell_names |
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.
use hasattr
not dir
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.
Small comment, otherwise lgtm
pandas/tests/io/excel/test_odf.py
Outdated
|
||
|
||
def test_read_newlines_between_xml_elements_table(): | ||
# Also test reading table from an text OpenDocument file |
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.
Please add gh reference with GH#number
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.
You don‘t need the comment, the reference is enough
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.
@phofl Done! But thanks! Good to know for my next PRs!
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 - pending @phofl's request.
…-dev#46050) * BUG: error in read_excel with some ods files pandas-dev#45598 * BUG: use hasattr instead of dir * DOC: add issue number in new test case * DOC: remove comment Co-authored-by: Dimitra Karadima <dkaradima@convertgroup.com>
doc/source/whatsnew/v1.5.0.rst
file if fixing a bug or adding a new feature.