Skip to content
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.

Sign up for GitHub

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

Jump to bottom

python: Use a context manager for opening files (SIM115) to solve some ResourceWarnings #4224

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

python: Use a context manager for opening files (SIM115) to solve some ResourceWarnings #4224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
261769d
grass.script.db: Close opened file in db_connection
echoix Aug 24, 2024
1ca0c19
t.unregister: Use a context manager for opening files (SIM115)
echoix Aug 24, 2024
7a98608
t.remove: Use a context manager for opening files (SIM115)
echoix Aug 24, 2024
7963689
t.rast.what: Use a context manager for opening files (SIM115)
echoix Aug 24, 2024
a31c297
grass.temporal.aggregation: Use a context manager for opening files (…
echoix Aug 24, 2024
bb2e999
Apply suggestions from code review
echoix Aug 26, 2024
f7a60ec
Update t.rast.what.py
echoix Aug 26, 2024
7c2df57
Merge branch 'main' into fix-resourcewarning-1
echoix Sep 7, 2024
53e28d7
Merge remote-tracking branch 'origin/main' into fix-resourcewarning-1
echoix Sep 18, 2024
6a751c9
Merge branch 'main' into fix-resourcewarning-1
echoix Oct 17, 2024
71bc611
Move line out of context manager
echoix Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions python/grass/script/db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,8 @@ def db_connection(force=False, env=None):
:return: parsed output of db.connect
""" # noqa: E501
try:
nuldev = open(os.devnull, "w")
conn = parse_command("db.connect", flags="g", stderr=nuldev, env=env)
nuldev.close()
with open(os.devnull, "w") as nuldev:
conn = parse_command("db.connect", flags="g", stderr=nuldev, env=env)
echoix marked this conversation as resolved.
Show resolved Hide resolved
except CalledModuleError:
conn = None

Expand Down
19 changes: 8 additions & 11 deletions python/grass/temporal/aggregation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,11 @@ def aggregate_raster_maps(

# Create the r.series input file
filename = gs.tempfile(True)
file = open(filename, "w")
with open(filename, "w") as out_file:
for name in inputs:
string = "%s\n" % (name)
out_file.write(string)

for name in inputs:
string = "%s\n" % (name)
file.write(string)

file.close()
# Run r.series
try:
if len(inputs) > 1000:
Expand Down Expand Up @@ -364,11 +362,10 @@ def aggregate_by_topology(
if len(aggregation_list) > 1:
# Create the r.series input file
filename = gs.tempfile(True)
file = open(filename, "w")
for name in aggregation_list:
string = "%s\n" % (name)
file.write(string)
file.close()
with open(filename, "w") as out_file:
for name in aggregation_list:
string = "%s\n" % (name)
out_file.write(string)

mod = copy.deepcopy(r_series)
mod(file=filename, output=output_name)
Expand Down
285 changes: 136 additions & 149 deletions temporal/t.rast.what/t.rast.what.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,78 +373,72 @@ def one_point_per_row_output(
output is of type: x,y,start,end,value
"""
# open the output file for writing
out_file = open(output, "w") if output != "-" else sys.stdout

if write_header is True:
out_str = ""
if vcat:
out_str += "cat{sep}"
if site_input:
out_str += "x{sep}y{sep}site{sep}start{sep}end{sep}value\n"
else:
out_str += "x{sep}y{sep}start{sep}end{sep}value\n"
out_file.write(out_str.format(sep=separator))

for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s") % (file_name))
map_list = output_time_list[count]
in_file = open(file_name)
for line in in_file:
line = line.split(separator)
with open(output, "w") if output != "-" else sys.stdout as out_file:
if write_header is True:
out_str = ""
if vcat:
cat = line[0]
x = line[1]
y = line[2]
values = line[4:]
if site_input:
site = line[3]
values = line[5:]

out_str += "cat{sep}"
if site_input:
out_str += "x{sep}y{sep}site{sep}start{sep}end{sep}value\n"
else:
x = line[0]
y = line[1]
if site_input:
site = line[2]
values = line[3:]
out_str += "x{sep}y{sep}start{sep}end{sep}value\n"
out_file.write(out_str.format(sep=separator))

for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s") % (file_name))
map_list = output_time_list[count]
with open(file_name) as in_file:
for line in in_file:
line = line.split(separator)
if vcat:
cat = line[0]
x = line[1]
y = line[2]
values = line[4:]
if site_input:
site = line[3]
values = line[5:]

for i in range(len(values)):
start, end = map_list[i].get_temporal_extent_as_tuple()
if vcat:
cat_str = "{ca}{sep}".format(ca=cat, sep=separator)
else:
cat_str = ""
if site_input:
coor_string = (
"%(x)10.10f%(sep)s%(y)10.10f%(sep)s%(site_name)s%(sep)s"
% (
else:
x = line[0]
y = line[1]
if site_input:
site = line[2]
values = line[3:]

for i in range(len(values)):
start, end = map_list[i].get_temporal_extent_as_tuple()
if vcat:
cat_str = "{ca}{sep}".format(ca=cat, sep=separator)
else:
cat_str = ""
if site_input:
coor_string = (
"%(x)10.10f%(sep)s%(y)10.10f%(sep)s%(site_name)s%(sep)s"
% (
{
"x": float(x),
"y": float(y),
"site_name": str(site),
"sep": separator,
}
)
)
else:
coor_string = "%(x)10.10f%(sep)s%(y)10.10f%(sep)s" % (
{"x": float(x), "y": float(y), "sep": separator}
)
time_string = "%(start)s%(sep)s%(end)s%(sep)s%(val)s\n" % (
{
"x": float(x),
"y": float(y),
"site_name": str(site),
"start": str(start),
"end": str(end),
"val": (values[i].strip()),
"sep": separator,
}
)
)
else:
coor_string = "%(x)10.10f%(sep)s%(y)10.10f%(sep)s" % (
{"x": float(x), "y": float(y), "sep": separator}
)
time_string = "%(start)s%(sep)s%(end)s%(sep)s%(val)s\n" % (
{
"start": str(start),
"end": str(end),
"val": (values[i].strip()),
"sep": separator,
}
)

out_file.write(cat_str + coor_string + time_string)

in_file.close()

if out_file is not sys.stdout:
out_file.close()
Comment on lines -446 to -447
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

out_file.write(cat_str + coor_string + time_string)


############################################################################
Expand All @@ -460,83 +454,80 @@ def one_point_per_col_output(
Each row represents a single raster map, hence a single time stamp
"""
# open the output file for writing
out_file = open(output, "w") if output != "-" else sys.stdout

first = True
for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s") % (file_name))
in_file = open(file_name)
lines = in_file.readlines()
with open(output, "w") if output != "-" else sys.stdout as out_file:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not fixed yet, I only solved the conflicts

for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s") % (file_name))
with open(file_name) as in_file:
lines = in_file.readlines()

matrix = []
for line in lines:
matrix.append(line.split(separator))
matrix = []
for line in lines:
matrix.append(line.split(separator))

num_cols = len(matrix[0])
num_cols = len(matrix[0])

if first is True:
if write_header is True:
out_str = "start%(sep)send" % ({"sep": separator})

# Define different separator for coordinates and sites
if separator == ",":
coor_sep = ";"
else:
coor_sep = ","
if first is True:
if write_header is True:
out_str = "start%(sep)send" % ({"sep": separator})

for row in matrix:
if vcat:
cat = row[0]
x = row[1]
y = row[2]
out_str += (
"{sep}{cat}{csep}{x:10.10f}{csep}"
"{y:10.10f}".format(
cat=cat,
x=float(x),
y=float(y),
sep=separator,
csep=coor_sep,
)
)
if site_input:
site = row[3]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)
# Define different separator for coordinates and sites
if separator == ",":
coor_sep = ";"
else:
x = row[0]
y = row[1]
out_str += "{sep}{x:10.10f}{csep}{y:10.10f}".format(
x=float(x), y=float(y), sep=separator, csep=coor_sep
)
if site_input:
site = row[2]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)
coor_sep = ","

for row in matrix:
if vcat:
cat = row[0]
x = row[1]
y = row[2]
out_str += (
"{sep}{cat}{csep}{x:10.10f}{csep}"
"{y:10.10f}".format(
cat=cat,
x=float(x),
y=float(y),
sep=separator,
csep=coor_sep,
)
)
if site_input:
site = row[3]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)
else:
x = row[0]
y = row[1]
out_str += "{sep}{x:10.10f}{csep}{y:10.10f}".format(
x=float(x), y=float(y), sep=separator, csep=coor_sep
)
if site_input:
site = row[2]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)

out_file.write(out_str + "\n")
out_file.write(out_str + "\n")

first = False
first = False

if vcat:
ncol = 4
else:
ncol = 3
for col in range(num_cols - ncol):
start, end = output_time_list[count][col].get_temporal_extent_as_tuple()
time_string = "%(start)s%(sep)s%(end)s" % (
{"start": str(start), "end": str(end), "sep": separator}
)
out_file.write(time_string)
for row in range(len(matrix)):
value = matrix[row][col + ncol]
out_file.write(
"%(sep)s%(value)s" % ({"sep": separator, "value": value.strip()})
if vcat:
ncol = 4
else:
ncol = 3
for col in range(num_cols - ncol):
start, end = output_time_list[count][col].get_temporal_extent_as_tuple()
time_string = "%(start)s%(sep)s%(end)s" % (
{"start": str(start), "end": str(end), "sep": separator}
)
out_file.write("\n")

in_file.close()
if out_file is not sys.stdout:
out_file.close()
out_file.write(time_string)
for row in range(len(matrix)):
value = matrix[row][col + ncol]
out_file.write(
"%(sep)s%(value)s"
% ({"sep": separator, "value": value.strip()})
)
out_file.write("\n")


############################################################################
Expand All @@ -553,7 +544,6 @@ def one_point_per_timerow_output(
3730731.49590371|5642483.51236521|6|8|7|7
3581249.04638104|5634411.97526282|5|8|7|7
""" # noqa: E501
out_file = open(output, "w") if output != "-" else sys.stdout

matrix = []
header = ""
Expand All @@ -563,7 +553,6 @@ def one_point_per_timerow_output(
file_name = output_files[count]
gs.verbose("Transforming r.what output file %s" % (file_name))
map_list = output_time_list[count]
in_file = open(file_name)

if write_header:
if first is True:
Expand All @@ -582,7 +571,8 @@ def one_point_per_timerow_output(
)
header += time_string

lines = in_file.readlines()
with open(file_name) as in_file:
lines = in_file.readlines()

for i in range(len(lines)):
cols = lines[i].split(separator)
Expand All @@ -602,26 +592,23 @@ def one_point_per_timerow_output(

first = False

in_file.close()

if write_header:
out_file.write(header + "\n")
with open(output, "w") if output != "-" else sys.stdout as out_file:
if write_header:
out_file.write(header + "\n")

gs.verbose(_("Writing the output file <%s>") % (output))
for row in matrix:
first = True
for col in row:
value = col.strip()
gs.verbose(_("Writing the output file <%s>") % (output))
for row in matrix:
first = True
for col in row:
value = col.strip()

if first is False:
out_file.write("%s" % (separator))
out_file.write(value)
if first is False:
out_file.write("%s" % (separator))
out_file.write(value)

first = False
first = False

out_file.write("\n")
if out_file is not sys.stdout:
out_file.close()
out_file.write("\n")


############################################################################
Expand Down
Loading
Loading