Skip to content

Conversation

twoertwein
Copy link
Member

@twoertwein twoertwein commented Aug 17, 2020

Needs tests!

@simonjayhawkins simonjayhawkins added the IO Excel read_excel, to_excel label Aug 17, 2020
@simonjayhawkins simonjayhawkins added this to the 1.1.1 milestone Aug 17, 2020
@simonjayhawkins
Copy link
Member

simonjayhawkins commented Aug 17, 2020

Needs tests!

There were changes to test_to_excel_output_encoding in #34464, can that test be updated to cover this.

@WillAyd
Copy link
Member

WillAyd commented Aug 17, 2020

Does this do nothing or am I misreading it? Doesn't the excel format already define how to interpret its data?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we need a test here; I know you are trying to get one @twoertwein

@twoertwein
Copy link
Member Author

twoertwein commented Aug 17, 2020

Does this do nothing or am I misreading it? Doesn't the excel format already define how to interpret its data?

I think it doesn't do what I expected it to do:

It will forward the encoding to

parser = TextParser(

but this is the wrong place (encoding finally ends up in TextFileReader but it seems to be never used there), it should (if we have an Excel file that needs a specific encoding) go to

return open_workbook(filepath_or_buffer)

since this is the place (or one of the places) where the file is actually opened. After that, all functions seem to operate on strings so there shouldn't be a need for an encoding form there one.

I will put this PR in draft mode until we have an excel file that needs an encoding.

Edit: closed, a future PR might go the route of forwarding the encoding to each "Excel backend" (if there is a need for that)

@twoertwein twoertwein marked this pull request as draft August 17, 2020 21:14
@twoertwein twoertwein closed this Aug 18, 2020
@twoertwein twoertwein deleted the excel branch October 29, 2020 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IO Excel read_excel, to_excel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: read_excel not accepting encoding on 1.1.0

4 participants