On 10/7/20 8:32 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Make the file handling here just a tiny bit more idiomatic.
>> (I realize this is heavily subjective.)
>>
>> Use exist_ok=True for os.makedirs and remove the exception,
>> use fdopen() to wrap the file descriptor in a File-like object,
>> and use a context manager for managing the file pointer.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> ---
>> scripts/qapi/gen.py | 25 +++++++++++--------------
>> 1 file changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index 3624162bb77..579ee283297 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -14,7 +14,6 @@
>> # See the COPYING file in the top-level directory.
>>
>> from contextlib import contextmanager
>> -import errno
>> import os
>> import re
>> from typing import (
>> @@ -67,21 +66,19 @@ def write(self, output_dir: str) -> None:
>> return
>> pathname = os.path.join(output_dir, self.fname)
>> odir = os.path.dirname(pathname)
>> +
>> if odir:
>> - try:
>> - os.makedirs(odir)
>> - except os.error as e:
>> - if e.errno != errno.EEXIST:
>> - raise
>> + os.makedirs(odir, exist_ok=True)
>
> I wouldn't call this part "heavily subjective". When wrote the old
> code, exist_ok was still off limits (it's new in 3.2).
>
It's cool if you agree, I just realize that what people consider
idiomatic is subjective unless it's enforcable by a tool. This isn't.
"I made this look more like if I wrote it, which caused Dopamine" is a
bad commit message. (But more true.)
>> +
>> + # use os.open for O_CREAT to create and read a non-existant file
>> fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
>> - f = open(fd, 'r+', encoding='utf-8')
>> - text = self.get_content()
>> - oldtext = f.read(len(text) + 1)
>> - if text != oldtext:
>> - f.seek(0)
>> - f.truncate(0)
>> - f.write(text)
>> - f.close()
>> + with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
>> + text = self.get_content()
>> + oldtext = fp.read(len(text) + 1)
>> + if text != oldtext:
>> + fp.seek(0)
>> + fp.truncate(0)
>> + fp.write(text)
>>
>>
>> def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
Thanks, though :)