Replace open+read/write with pathlib's read/write_text/bytes #3263
Conversation
happz
added
code | style
| with open(ref_filepath, encoding='utf-8') as datafile: | ||
| data = tmt.utils.yaml_to_dict(datafile.read()) | ||
| data = tmt.utils.yaml_to_dict(ref_filepath.read_text(encoding='utf-8')) | ||
|
|
There was a problem hiding this comment.
I believe utf8 is default, so could be dropped since we're "modernizing"?
There was a problem hiding this comment.
I’d love to, but I’m not sure about the default:
… the contents of the file are returned as str, the bytes having been first decoded using a platform-dependent encoding or using the specified encoding if given.
It may not necessarily be utf-8, IIUIC. I’m not sure what’s better, whether we should leave it to OS and Python to pick a sane default, or whether we should enforce the default as we see it, or whether we should always enforce UTF-8, without any exception. I also think we still ignore the problem of locales when parsing output of various commands, too, so far we were able to get away with it. Seems like we have several topics to discuss :)
There was a problem hiding this comment.
Yeah, I believe another patch or discussion topic rather?
happz
requested review from
frantisekz,
psss,
lukaszachy,
thrix and
janhavlin
as code owners
tmt/utils/__init__.py
Outdated
| except OSError as error: | ||
| raise FileError(f"Failed to write '{path}'.\n{error}") | ||
| raise FileError("Failed to write into '{path}' file.") from error |
There was a problem hiding this comment.
@happz why is f-string not needed here?
tmt/utils/__init__.py
Outdated
| hash=hash_value, | ||
| hashtype=used_hash.lower() | ||
| ), source_name)) | ||
| for line in (cwd / self.sources_file_name).read_text().splitlines(): |
There was a problem hiding this comment.
Seems there is a pattern here, could we add
def read_lines(path: Path) -> list[str]:
return path.read_text().splitlines()
To our utils? So it would become:
for line in read_path_lines(cwd / self.sources_file_name):
There was a problem hiding this comment.
Hmm, if nothing else, for line in sys.stdin works. No idea whether pathlib has some alternative, maybe we can add a similar iterator to our custom class.
There was a problem hiding this comment.
Added in 187f79d. Iterator is not possible because both read_text() and splitlines() have their own options.
happz
force-pushed
the
drop-open-fot-read-write-text-bytes
branch
from
9e9e0be
to
187f79d
Compare
Pull Request Checklist