Re: [PATCH v5 30/36] qapi/gen.py: update write() to be more idiomatic

[John Snow]

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 :)