python: Use a context manager for opening files (SIM115) to solve some ResourceWarnings #4224
Conversation
github-actions
bot
added
temporal
temporal/t.rast.what/t.rast.what.py
Outdated
| in_file.close() | ||
| if out_file is not sys.stdout: | ||
| out_file.close() | ||
| in_file.close() |
There was a problem hiding this comment.
It's kind of hard to review this due to the changed indentation, but shouldn't this be removed?
There was a problem hiding this comment.
Yes. At first I wasn't as sure, but in the 3 later PRs (not submitted here yet), I've been doing it, after having tried it more.
Unfortunately, there will be a lots of places in future PRs where the indentation will change. Using side by side comparaison instead of inline would be easier to follow. I am trying to avoid just indenting everything. There are simple cases where a Path(a_file_var).read_text() can be used instead, and I try to use it first.
(See example in https://docs.astral.sh/ruff/rules/read-whole-file/ and https://docs.astral.sh/ruff/rules/write-whole-file/).
Some places I try to just change the order if there are no dependencies so the with is on its own without having to change the whole indent. It's difficult when it's used way below afterwards (like stds_export in a future PR, the diff is a little harder to follow).
There was a problem hiding this comment.
What about refactoring some of the functionality out to functions first so that the investment into the review is for the refactoring rather than just with statement? (Probably as a PR separate from this one.)
There was a problem hiding this comment.
Also, I'm for applying any "read/write whole file" changes first before this PR.
There was a problem hiding this comment.
Also, I'm for applying any "read/write whole file" changes first before this PR.
The ones that could be detected automatically were done in #4047 in mid July. The rule is active since so no more new introductions can happen.
It's only after some manual refactorings that I saw new occurrences appear.
There was a problem hiding this comment.
What about refactoring some of the functionality out to functions first so that the investment into the review is for the refactoring rather than just with statement? (Probably as a PR separate from this one.)
You're aware that there are 368+ places where there's missing a context manager for opened files? It's something a bit big... When the goal is to reduce some new Pylint errors, and help introducing pytest on windows....
There was a problem hiding this comment.
Can we make a plan how to handle all those? It will be a lot to review even if the changes would be trivial.
If the edits are automated and 1:1, then we can say we will review only a sample. What I as suggesting before is that we could treat that based on categories: dev/null group, stdout group, pathlib one-liners, huge changes which may as well be refactoring, ...
There was a problem hiding this comment.
They are not automated, yet. The patterns are easy to apply, so in theory there is nothing that would prevent ruff to create a fix in the future, buts it's just not done yet.
It ends up being a little keyboard dance, so it's not that long to do.
temporal/t.rast.what/t.rast.what.py
Outdated
| in_file.close() | ||
| if out_file is not sys.stdout: | ||
| out_file.close() | ||
| in_file.close() |
There was a problem hiding this comment.
What about refactoring some of the functionality out to functions first so that the investment into the review is for the refactoring rather than just with statement? (Probably as a PR separate from this one.)
temporal/t.rast.what/t.rast.what.py
Outdated
| in_file.close() | ||
| if out_file is not sys.stdout: | ||
| out_file.close() | ||
| in_file.close() |
There was a problem hiding this comment.
Also, I'm for applying any "read/write whole file" changes first before this PR.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| if out_file is not sys.stdout: | ||
| out_file.close() |
There was a problem hiding this comment.
Is there a way to have a test that triggers this? When reading carefully my code coverage equivalent branch, the t.rast.what file was only imported. I think it's just a collection problem, as there is a test using stdout and doesn't specify layout, so row will be used, entering in this function.
I want to know what happens when having a sys.stdout used as with a context manager, does it close it when exiting the context? It didn't break any tests though.
To be sure of the syntax, in the with of out_file above, I used way more parentheses, and let black simplify them back. I also checked with small examples in the same function above that pyright detected usages that allow or not with a context manager, and sys.stdout can be used with a context manager. It works just like text files. Can it really be closed?
There was a problem hiding this comment.
There are three tests in test_what.py which test with output="-", so that's likely testing this stdout case.
I didn't check, but I would guess that you can close stdout, but you don't want to because any code later can be "surprised" by that.
| matrix = [] | ||
| for line in lines: | ||
| matrix.append(line.split(separator)) | ||
| with open(output, "w") if output != "-" else sys.stdout as out_file: |
There was a problem hiding this comment.
Python code:
import sys
with sys.stdout as f:
f.write("Hello\n")
sys.stdout.write("World\n")produces:
Hello
Traceback (most recent call last):
File "/tmp/test.py", line 4, in <module>
sys.stdout.write("World\n")
ValueError: I/O operation on closed file.
And I think that will be unexpected for anyone who will touch this code later.
There was a problem hiding this comment.
It's not fixed yet, I only solved the conflicts
|
I'll take a new look at my next available coding time. Since that time, I've found other ways to not have to indent everything, by moving the static contents up and keeping the part that has to be indented small. Otherwise, it might be easier to look at the diff side by side rather than inline, and VS Code with whitespace highlighting is really good at seing that there are literally no change apart 4 spaces + black. |
| if file: | ||
| fd = open(file) | ||
|
|
||
| line = True |
There was a problem hiding this comment.
I have no idea what the line = True is here for. Is the value ever used?
There was a problem hiding this comment.
Probably so that it is initialized inside the whole loop? But it shouldn't work as expected, as it is being set by readlines()
|
This PR is not ready yet, there's still the stdout handling to go through |
This PR starts fixing some ResourceWarnings. I'm starting small, I am juggling with 4 sets of changes that are tested individually and one on top of the others, plus on top of my coverage branch. There are at least 368 places where a context manager should be used to open and close files. It shouldn't be done in one PR.
This first set is ready to be merged. It already solves some resource warnings appearing in macOS Pytest jobs that I was seeing.
Later on, I will merge some changes, but will explicitly make sure that changes in an input/output pair are in separate PRs. For example, if stds_export is changed, stds_import will be in another one. For changes in gunittest, it will be on their own. That way, it will help isolate the possibility of changing the code and the code treating the expected value, and will help make sure the existing code works well with only this part of the changes.