Support openpyxl 3.x #779
Support openpyxl 3.x #779
Conversation
| @@ -252,4 +249,9 @@ def __call__(self): | |||
| "Content-Type", | |||
| "application/vnd.openxmlformats-" "officedocument.spreadsheetml.sheet", | |||
| ) | |||
| return save_virtual_workbook(book) | |||
|
|
|||
| with NamedTemporaryFile(delete=False) as tmp: | |||
There was a problem hiding this comment.
It seems strange to me that we have delete False.
There was a problem hiding this comment.
It's modelled after this example: https://docs.python.org/3/library/tempfile.html#examples
Note that on python 3.13 the parameter is delete_on_close which is a more accurate name.
There was a problem hiding this comment.
Fine for the example, but who is taking care of cleaning the tmp file?
There was a problem hiding this comment.
The context manager is doing that. The parameter only prevents deletion when close() is called. When the context is exited the file is deleted anyway.
The documentation is a bit ambiguous, but that's how I understand it.
If delete is true (the default), the file is deleted as soon as it is closed.
https://docs.python.org/3.11/library/tempfile.html#tempfile.NamedTemporaryFile
In 3.13 there is an improvement:
If delete is true and delete_on_close is false, the file is deleted on context manager exit only
https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile
There was a problem hiding this comment.
Meh... I tested it - the file is not cleaned up by the context manager. I'll change the code.
| @@ -252,4 +249,9 @@ def __call__(self): | |||
| "Content-Type", | |||
| "application/vnd.openxmlformats-" "officedocument.spreadsheetml.sheet", | |||
| ) | |||
| return save_virtual_workbook(book) | |||
|
|
|||
| with NamedTemporaryFile(delete=False) as tmp: | |||
There was a problem hiding this comment.
| with NamedTemporaryFile(delete=False) as tmp: | |
| with NamedTemporaryFile() as tmp: |
| tmp.close() | ||
| with open(tmp.name, mode="rb") as f: | ||
| return f.read() |
There was a problem hiding this comment.
| tmp.close() | |
| with open(tmp.name, mode="rb") as f: | |
| return f.read() | |
| tmp.seek(0) | |
| return f.read() |
I am not really sure if the tmp.seek(0) is needed, but closing and reopening the file feels quite wrong.
There was a problem hiding this comment.
It feels right to me, because we're first writing and then reading. That's different modes.
|
@reinhardt why did you merge this, it was not approved and I asked for changes... |
| @@ -218,11 +219,7 @@ def create_workbook(self): | |||
| value = module.title | |||
| if value is not None: | |||
| cell = sheet.cell(row=row, column=column) | |||
| if key == "number": | |||
| # force sting | |||
| cell.set_explicit_value(value) | |||
|
I did implement all your requested changes. |
Note true: Please complete the work and next time wait for an approval |
|
GHA tests for python 3.11 are here: #780 This was added after the PR was merged. That said, I can look into it again, but I doubt that it's needed. The method |
They are failing.
That means we use the default. I already checked that yesterday. |
|
We don't need the case distiction. |
No description provided.