Skip to content
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: Fix date_format and datetime_format arguments to ExcelWriter being ignored #47315

Closed

Conversation

TimoMorris
Copy link

Notes:

  • The fix is as suggested in the latest comment on the issue.
  • I applied the fix to all the writers, not just Openpyxl. This was following the pattern of changes made in the PR where the bug was introduced: DEPR: **kwargs in ExcelWriter #40430
  • The fix is very straightforward (2-line change in each of 3 files); the challenge was adding tests for it, since to properly test it we need to inspect the formatting info on the Excel file. Crucially, there is not a common API for this within pandas across the different engines, so initially adding the test into the TestExcelWriter class, which is parametrized over the different writers, gave some quite awkward code branching between the different APIs for the different engines. So I decided to move the tests to the test files for each individual engine, though this leaves some duplication between them.

Potential outstanding issues:

  • Moving the test from TextExcelWriter to the individual test files meant losing the openpyxl-.xlsm parameter, since there didn't seem to be a file where this extension was being tested.
  • Even with the arguments being passed through, ODSWriter does not seem to respect them, and this may be a limitation of the ODF specification. This felt like it was out-of-scope for this bug. Consequently, there was not a test added for the ODF engine.

Swap rows and columns so that columns are homogenous and dates aren't converted to datetimes (i.e. datetime64[ns]) on initialisation of df. Clarify scope of test.
Avoid messy branching within test due to handling different engines in parametrization on TestExcelWriter class. Different engines have different APIs for accessing formatting info so probably better as tests in per-engine test files, even though this entails some duplication across files.
@TimoMorris TimoMorris changed the title Fix date_format and datetime_format arguments to ExcelWriter being ignored BUG: Fix date_format and datetime_format arguments to ExcelWriter being ignored Jun 12, 2022
pandas/tests/io/excel/test_openpyxl.py Outdated Show resolved Hide resolved
pandas/tests/io/excel/test_writers.py Outdated Show resolved Hide resolved
@datapythonista datapythonista added Bug IO Excel read_excel, to_excel labels Jun 16, 2022
@TimoMorris TimoMorris requested a review from phofl June 21, 2022 20:16

def test_write_date_datetime_format(ext):
# see gh-44284
xlrd = pytest.importorskip("xlrd")
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed. It's already called near the top of the file

@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Aug 21, 2022
@mroeschke
Copy link
Member

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IO Excel read_excel, to_excel Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: date_format and datetime_format arguments to ExcelWriter are currently ignored by default openpyxl engine
4 participants