diff mbox series

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

Message ID 20201005195158.2348217-31-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC
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(-)

Comments

Markus Armbruster Oct. 7, 2020, 12:32 p.m. UTC | #1
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).

> +
> +        # 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>
John Snow Oct. 7, 2020, 4:25 p.m. UTC | #2
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 :)
diff mbox series

Patch

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