-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: Use a context manager for opening files #219
fix: Use a context manager for opening files #219
Conversation
wasm_file = NamedTemporaryFile(suffix=".wasm", delete=False) | ||
with ( | ||
NamedTemporaryFile(suffix=".qasm", delete=False) as qasm_file, | ||
NamedTemporaryFile(suffix=".wasm", delete=False) as wasm_file, |
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.
It's not obvious to me if we need delete=False
anymore. See https://docs.python.org/3.11/library/tempfile.html#tempfile.NamedTemporaryFile
On Python 3.12 onward, there is a new parameter called delete_on_close
that is preferable here:
Therefore to use the name of the temporary file to reopen the file after closing it, either make sure not to delete the file upon closure (set the delete parameter to be false) or, in case the temporary file is created in a with statement, set the delete_on_close parameter to be false.
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.
Perhaps @neal-erickson might have insight about why delete=False
was needed?
…er-test_pytket_to_phir_no_machine_at_all
Hi @peter-campora, would it possible for you to try this branch on Windows? @neal-erickson shared:
I do see the CI on windows is passing fully, so I am unsure if there is any problem still unfixed on Windows. |
I forget what the status of python tooling natively in my windows environment is exactly, but I can take a look if still desired that I do some testing |
If you don't mind, it will be helpful to eliminate the possibility that this is still an issue. |
Going to merge this. If it still fails on Windows, we will fix it before the next release. |
Description
ruff
pointed out errors like the following:Turns out resolving these also fixes #108.
Additionally:
auto_rebase_pass()
method is deprecated and will be removed after pytket 1.33, please useAutoRebase
instead."Type of change
Checklist