Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pass through Engine kwargs in ExcelWriter #43445
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
Pass through Engine kwargs in ExcelWriter #43445
Changes from 11 commits
b18af6f
7c439c8
d214487
e7c2b6e
a4a29ee
44653f9
c94561b
02406a6
4708e3e
fb949f5
916f077
97589ea
8a7e79d
5da5254
f90e7db
f8a8fae
87bc0e7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think read_only would be sufficient. Need to verify the kwarg gets through.
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 thought testing for all available parameters would be nice, since then we will also be warned when API changes on openpyxl's side...
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.
Actually, read-only will not work here, and give different errors on windows than linux, this is why the previous test failed
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.
If I'm understanding the test correctly, the passing of kwargs through to openpyxl could be entirely removed from pandas and this test would pass. Let me know if you're not sure of a way to improve that (or if my assessment is wrong).
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.
hmm, that's true. That's why I have the other test:
test_engine_kwargs_append_invalid
that tests for an invalid keyword argument inload_workbook
. Also the other testtest_engine_kwargs_append_data_only
tests for correct use of openpyxl'sdata_only
being passed through. That would make this test redundant indeed? It just felt logical to first test if passing an engine kwarg does not raise an error normally, then test if it does raise with an invalid argument and then test if the passing of engine kwargs works well together with openpyxl. Then in that order, people will know more precisely where the error lies.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.
Tell me if you want it removed!
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.
Thanks for the summary, very helpful. Yes, I think this test can be removed as it's redundant with
test_engine_kwargs_append_data_only
. In general, we'd rather not have to change tests if the API of a third party engine changes and it doesn't negatively impact pandas. It is okay to do so when necessary, as is in the data_only test.