-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: excelwriter engine_kwargs fix #42214
Conversation
Hello @joeperdefloep! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-07-12 21:54:21 UTC |
We would really love to have this fix in the next version of pandas, since we have to mitigate XLSX injections. Is there something I could help here to achieve this? |
@joeperdefloep can you merge master |
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 PR!
@@ -199,7 +199,7 @@ def __init__( | |||
engine_kwargs=engine_kwargs, | |||
) | |||
|
|||
self.book = Workbook(self.handles.handle, **engine_kwargs) | |||
self.book = Workbook(self.handles.handle, engine_kwargs) |
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.
Can you add a test for this.
... with ExcelWriter( | ||
... "path_to_file.xlsx", | ||
... engine_kwargs={"strings_to_formulas":False} | ||
... ) as writer: |
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 this will only be valid with xlsxwriter, is that right? Can you specify the engine here to make it clear that's what is being used.
@@ -64,6 +64,7 @@ def test_write_append_mode_raises(ext): | |||
ExcelWriter(f, engine="xlsxwriter", mode="a") | |||
|
|||
|
|||
|
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.
Remove this blank line, it will not pass code checks (two blank lines between functions)
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
@joeperdefloep do you still want to work on this? |
Thanks for the PR, but it appears to have gone stale. Let us know if you're still interested in working on this and we can reopen. Closing. |
@mattelacchiato - any interest in picking this up? |
Heyy, sorry I was on holidays a bit so that's why it stopped and went stale... Also there was a weird thing that there was a merge conflict, where my test was changed to another one. That's why this PR doesn't mention it. I will be at my computer tomorrow and hopefully will come back to this this week :) |
The
engine_kwargs
argument was not passed properly and this was not tested. Also added some documentation.