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

REGR: ExcelWriter fails when passed kwargs #42292

Merged
merged 3 commits into from
Jun 29, 2021

Conversation

rhshadrach
Copy link
Member

@rhshadrach rhshadrach commented Jun 29, 2021

Seeing some open resource test failure in excel writer, but not sure why. Checking if they occur on CI too.

Before this PR, I didn't notice that only xlsxwriter actually utilizes the kwargs/engine_kwargs. This makes the non-xlsxwriter tests slightly awkward - we pass the kwargs and just make sure the call to ExcelWriter doesn't fail because I don't see anything else to check.

@rhshadrach rhshadrach changed the title Excel kwargs regr REGR: ExcelWriter fails when passed kwargs Jun 29, 2021
@rhshadrach rhshadrach added IO Excel read_excel, to_excel Regression Functionality that used to work in a prior pandas version labels Jun 29, 2021
@rhshadrach rhshadrach added this to the 1.3 milestone Jun 29, 2021
@rhshadrach
Copy link
Member Author

The open resource failures were from test_kwargs_deprecated, which is an invalid test. The try-except in that test catching the TypeError, which was supposed to be raised due to the invalid kwarg, was instead covering up this regression. The fix for the regression in this PR then made the test in question fail when creating the workbook for xlsxwriter, leading to the open resource error.

The fix for this is to break up the test for each engine, supplying valid kwargs (particularly, for xlsxwriter). Since this was already done in this PR, I've simply removed the invalid test.

@simonjayhawkins simonjayhawkins mentioned this pull request Jun 29, 2021
@jreback jreback merged commit 8d5d67b into pandas-dev:master Jun 29, 2021
@jreback
Copy link
Contributor

jreback commented Jun 29, 2021

thanks @rhshadrach very nice. is there an issue to remove this for 1.4?

@jreback
Copy link
Contributor

jreback commented Jun 29, 2021

@meeseeksdev backport 1.3.x

@lumberbot-app
Copy link

lumberbot-app bot commented Jun 29, 2021

Something went wrong ... Please have a look at my logs.

simonjayhawkins pushed a commit that referenced this pull request Jun 29, 2021
Co-authored-by: Richard Shadrach <45562402+rhshadrach@users.noreply.github.com>
@rhshadrach
Copy link
Member Author

is there an issue to remove this for 1.4?

For 2.0 - yes, it's part of #30228

@rhshadrach rhshadrach deleted the excel_kwargs_regr branch July 1, 2021 00:58
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
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 Regression Functionality that used to work in a prior pandas version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REGR: ExcelWriter fails when supplying kwargs
2 participants