Use this in preference to 'None', which helps remove some edge cases in
the typing.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index a6dc991b1d03..0c5d1fee6088 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -245,23 +245,23 @@ def __init__(self,
         self._pydoc = pydoc
         self._genc: Optional[QAPIGenC] = None
         self._genh: Optional[QAPIGenH] = None
-        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
+        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
         self._main_module: Optional[str] = None
 
     @staticmethod
-    def _is_user_module(name: Optional[str]) -> bool:
-        return bool(name and not name.startswith('./'))
+    def _is_user_module(name: str) -> bool:
+        return not name.startswith('./')
 
     @staticmethod
-    def _is_builtin_module(name: Optional[str]) -> bool:
-        return not name
+    def _is_builtin_module(name: str) -> bool:
+        return name == './builtin'
 
-    def _module_dirname(self, name: Optional[str]) -> str:
+    def _module_dirname(self, name: str) -> str:
         if self._is_user_module(name):
             return os.path.dirname(name)
         return ''
 
-    def _module_basename(self, what: str, name: Optional[str]) -> str:
+    def _module_basename(self, what: str, name: str) -> str:
         ret = '' if self._is_builtin_module(name) else self._prefix
         if self._is_user_module(name):
             basename = os.path.basename(name)
@@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
             if name != self._main_module:
                 ret += '-' + os.path.splitext(basename)[0]
         else:
-            name = name[2:] if name else 'builtin'
-            ret += re.sub(r'-', '-' + name + '-', what)
+            ret += re.sub(r'-', '-' + name[2:] + '-', what)
         return ret
 
-    def _module_filename(self, what: str, name: Optional[str]) -> str:
+    def _module_filename(self, what: str, name: str) -> str:
         return os.path.join(self._module_dirname(name),
                             self._module_basename(what, name))
 
-    def _add_module(self, name: Optional[str], blurb: str) -> None:
+    def _add_module(self, name: str, blurb: str) -> None:
         basename = self._module_filename(self._what, name)
         genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
         genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
@@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
             self._main_module = name
         self._add_module(name, blurb)
 
-    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
-        self._add_module(name and './' + name, blurb)
+    def _add_system_module(self, name: str, blurb: str) -> None:
+        self._add_module(f"./{name}", blurb)
 
     def write(self, output_dir: str, opt_builtins: bool = False) -> None:
         for name in self._module:
@@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
     def visit_module(self, name: Optional[str]) -> None:
         if name is None:
             if self._builtin_blurb:
-                self._add_system_module(None, self._builtin_blurb)
+                self._add_system_module('builtin', self._builtin_blurb)
                 self._begin_system_module(name)
             else:
                 # The built-in module has not been created.  No code may
-- 
2.26.2

John Snow <jsnow@redhat.com> writes:

> Use this in preference to 'None', which helps remove some edge cases in
> the typing.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Clearly better.  Should've done it this way in commit c2e196a9b4 "qapi:
Prepare for system modules other than 'builtin'".

> ---
>  scripts/qapi/gen.py | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index a6dc991b1d03..0c5d1fee6088 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -245,23 +245,23 @@ def __init__(self,
>          self._pydoc = pydoc
>          self._genc: Optional[QAPIGenC] = None
>          self._genh: Optional[QAPIGenH] = None
> -        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>          self._main_module: Optional[str] = None
>  
>      @staticmethod
> -    def _is_user_module(name: Optional[str]) -> bool:
> -        return bool(name and not name.startswith('./'))
> +    def _is_user_module(name: str) -> bool:
> +        return not name.startswith('./')
>  
>      @staticmethod
> -    def _is_builtin_module(name: Optional[str]) -> bool:
> -        return not name
> +    def _is_builtin_module(name: str) -> bool:
> +        return name == './builtin'
>  
> -    def _module_dirname(self, name: Optional[str]) -> str:
> +    def _module_dirname(self, name: str) -> str:
>          if self._is_user_module(name):
>              return os.path.dirname(name)
>          return ''
>  
> -    def _module_basename(self, what: str, name: Optional[str]) -> str:
> +    def _module_basename(self, what: str, name: str) -> str:
>          ret = '' if self._is_builtin_module(name) else self._prefix
>          if self._is_user_module(name):
>              basename = os.path.basename(name)
> @@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
>              if name != self._main_module:
>                  ret += '-' + os.path.splitext(basename)[0]
>          else:

Possible drive-by improvement:

               assert name.startswith('./')

> -            name = name[2:] if name else 'builtin'
> -            ret += re.sub(r'-', '-' + name + '-', what)
> +            ret += re.sub(r'-', '-' + name[2:] + '-', what)
>          return ret
>  
> -    def _module_filename(self, what: str, name: Optional[str]) -> str:
> +    def _module_filename(self, what: str, name: str) -> str:
>          return os.path.join(self._module_dirname(name),
>                              self._module_basename(what, name))
>  
> -    def _add_module(self, name: Optional[str], blurb: str) -> None:
> +    def _add_module(self, name: str, blurb: str) -> None:
>          basename = self._module_filename(self._what, name)
>          genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>          genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> @@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
>              self._main_module = name
>          self._add_module(name, blurb)
>  
> -    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
> -        self._add_module(name and './' + name, blurb)
> +    def _add_system_module(self, name: str, blurb: str) -> None:
> +        self._add_module(f"./{name}", blurb)

I like f-strings, I really do, but is

    f"./{name}"

really clearer than

    './' + name

?

>  
>      def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>          for name in self._module:
> @@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
>      def visit_module(self, name: Optional[str]) -> None:
>          if name is None:
>              if self._builtin_blurb:
> -                self._add_system_module(None, self._builtin_blurb)
> +                self._add_system_module('builtin', self._builtin_blurb)
>                  self._begin_system_module(name)
>              else:
>                  # The built-in module has not been created.  No code may

On 12/16/20 3:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Use this in preference to 'None', which helps remove some edge cases in
>> the typing.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Clearly better.  Should've done it this way in commit c2e196a9b4 "qapi:
> Prepare for system modules other than 'builtin'".
> 
>> ---
>>   scripts/qapi/gen.py | 27 +++++++++++++--------------
>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index a6dc991b1d03..0c5d1fee6088 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -245,23 +245,23 @@ def __init__(self,
>>           self._pydoc = pydoc
>>           self._genc: Optional[QAPIGenC] = None
>>           self._genh: Optional[QAPIGenH] = None
>> -        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
>> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>>           self._main_module: Optional[str] = None
>>   
>>       @staticmethod
>> -    def _is_user_module(name: Optional[str]) -> bool:
>> -        return bool(name and not name.startswith('./'))
>> +    def _is_user_module(name: str) -> bool:
>> +        return not name.startswith('./')
>>   
>>       @staticmethod
>> -    def _is_builtin_module(name: Optional[str]) -> bool:
>> -        return not name
>> +    def _is_builtin_module(name: str) -> bool:
>> +        return name == './builtin'
>>   
>> -    def _module_dirname(self, name: Optional[str]) -> str:
>> +    def _module_dirname(self, name: str) -> str:
>>           if self._is_user_module(name):
>>               return os.path.dirname(name)
>>           return ''
>>   
>> -    def _module_basename(self, what: str, name: Optional[str]) -> str:
>> +    def _module_basename(self, what: str, name: str) -> str:
>>           ret = '' if self._is_builtin_module(name) else self._prefix
>>           if self._is_user_module(name):
>>               basename = os.path.basename(name)
>> @@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
>>               if name != self._main_module:
>>                   ret += '-' + os.path.splitext(basename)[0]
>>           else:
> 
> Possible drive-by improvement:
> 
>                 assert name.startswith('./')
> 

As long as nobody tells me to split that unrelated change into a new 
patch I'll happily add drive-by improvements upon request ;)

>> -            name = name[2:] if name else 'builtin'
>> -            ret += re.sub(r'-', '-' + name + '-', what)
>> +            ret += re.sub(r'-', '-' + name[2:] + '-', what)
>>           return ret
>>   
>> -    def _module_filename(self, what: str, name: Optional[str]) -> str:
>> +    def _module_filename(self, what: str, name: str) -> str:
>>           return os.path.join(self._module_dirname(name),
>>                               self._module_basename(what, name))
>>   
>> -    def _add_module(self, name: Optional[str], blurb: str) -> None:
>> +    def _add_module(self, name: str, blurb: str) -> None:
>>           basename = self._module_filename(self._what, name)
>>           genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>>           genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
>> @@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
>>               self._main_module = name
>>           self._add_module(name, blurb)
>>   
>> -    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
>> -        self._add_module(name and './' + name, blurb)
>> +    def _add_system_module(self, name: str, blurb: str) -> None:
>> +        self._add_module(f"./{name}", blurb)
> 
> I like f-strings, I really do, but is
> 
>      f"./{name}"
> 
> really clearer than
> 
>      './' + name
> 
> ?
> 

I tend to avoid string concatenation as a habit, but there's no greater 
reason; the old value needed updating and that's what fell out of my 
keyboard, I suppose.

>>   
>>       def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>>           for name in self._module:
>> @@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
>>       def visit_module(self, name: Optional[str]) -> None:
>>           if name is None:
>>               if self._builtin_blurb:
>> -                self._add_system_module(None, self._builtin_blurb)
>> +                self._add_system_module('builtin', self._builtin_blurb)
>>                   self._begin_system_module(name)
>>               else:
>>                   # The built-in module has not been created.  No code may