-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: to_xml
with index=False
and offset input index
#42464
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
ff3f927
to
6dcba15
Compare
Fixes pandas-dev#42458 It was assumed that the index contains the element `0`. This led to a defect when the index of the input Dataframe has an offset, which is a common use case when streaming Dataframes via generators. This fix consists of not relying on accessing the `0` element of `frame_dicts`.
6dcba15
to
170ca23
Compare
cc @ParfaitG if you can have a look here |
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.
Good catch on this issue and good work with this fix, @stephan-hesselmann-by! I have minor comments.
@@ -290,6 +293,42 @@ def test_index_false_rename_row_root(datapath, parser): | |||
assert output == expected | |||
|
|||
|
|||
def test_index_false_with_offset_input_index(parser): | |||
""" |
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.
Not sure we annotate reason for test even on bug fixes.
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 can remove the docstring if it goes against coding guidelines for the project. But personally I like to give context to tests like these.
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.
we normally don't add this, just a comment to the issue. but no harm as you have written it.
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! Thanks, @stephan-hesselmann-by!
thanks @stephan-hesselmann-by for the patch and @ParfaitG for the review! |
@meeseeksdev backport 1.3.x |
…fset input index
Something went wrong ... Please have a look at my logs. |
… index (#42513) Co-authored-by: Stephan Heßelmann <[email protected]>
Fixes #42458
It was assumed that the index contains the element
0
. This led to adefect when the index of the input Dataframe has an offset, which is a
common use case when streaming Dataframes via generators.
This fix consists of not relying on accessing the
0
element offrame_dicts
.to_xml
raisesKeyError
forindex=False
when the index does not start at zero #42458