Skip to content

Ods loses spaces 32207 #33233

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

Merged
merged 7 commits into from
Apr 6, 2020
Merged

Conversation

detrout
Copy link
Contributor

@detrout detrout commented Apr 2, 2020

  • closes read_excel loses spaces on ods #32207
  • tests added / passed tests.io.excel.test_reader_spaces
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff (I had to use git diff master though)
  • whatsnew entry. It's only a bug fix does it need a whatsnew?

I'm not sure how you make xlsxb files so that test wasn't implement when checking for the different types of space.

There's quite possibly many other ways this parser doesn't handle the odf specification.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

just pushed an .xlsb copy up; did it with a "Save As" from microsoft excel not sure if supported by other tools

@WillAyd WillAyd added the IO Excel read_excel, to_excel label Apr 2, 2020
@WillAyd WillAyd added this to the 1.1 milestone Apr 2, 2020
@WillAyd
Copy link
Member

WillAyd commented Apr 2, 2020

Can you also add a whatsnew for 1.1?

@WillAyd
Copy link
Member

WillAyd commented Apr 2, 2020

Can you run black on the changes? Should fix the code checks failure

The linux 37 failure is probably unrelated

@detrout
Copy link
Contributor Author

detrout commented Apr 2, 2020

While I was filling out the form I ran black, but forgot to push the commit.

And thanks for uploading a xlsb file, I don't have a copy of excel here at home.

@detrout detrout force-pushed the ods-loses-spaces-32207 branch from 43f8e27 to dba5571 Compare April 3, 2020 02:56
@detrout detrout force-pushed the ods-loses-spaces-32207 branch from dba5571 to fdda928 Compare April 3, 2020 02:58
@WillAyd
Copy link
Member

WillAyd commented Apr 3, 2020

If you can add a whatsnew note for 1.1 this lgtm - thanks!

@detrout
Copy link
Contributor Author

detrout commented Apr 3, 2020

How's the description?

@WillAyd
Copy link
Member

WillAyd commented Apr 3, 2020

@jreback care to look?

@jreback
Copy link
Contributor

jreback commented Apr 3, 2020

looks ok, i imagine this would slow down writing a bit?

@WillAyd
Copy link
Member

WillAyd commented Apr 3, 2020

Hmm fair point. @detrout do you mind run asv continuous upstream/master -b io.excel.ReadExcel from the asv_bench directory and posting output here?

Yea in any case fixes a bug so probably not a blocker, but might want to be cognizant of perf impact(s) if any

@detrout
Copy link
Contributor Author

detrout commented Apr 3, 2020

looks ok, i imagine this would slow down writing a bit?

There's no OpenDocument write support so I don't think it can slow down writes.
I'll need to figure out how to have a stable test environment for running a benchmark

@jreback jreback merged commit f404a3f into pandas-dev:master Apr 6, 2020
@jreback
Copy link
Contributor

jreback commented Apr 6, 2020

thanks @detrout

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.

read_excel loses spaces on ods
3 participants