The edge case is that if the name is '', this expression returns a
string instead of a bool, which violates our declared type.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qapi/gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index f2e2746fea5..1bad37fc06b 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -243,7 +243,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
 
     @staticmethod
     def _is_user_module(name):
-        return name and not name.startswith('./')
+        return bool(name and not name.startswith('./'))
 
     @staticmethod
     def _is_builtin_module(name):
-- 
2.26.2

On 10/5/20 9:51 PM, John Snow wrote:
> The edge case is that if the name is '', this expression returns a
> string instead of a bool, which violates our declared type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  scripts/qapi/gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

John Snow <jsnow@redhat.com> writes:

> The edge case is that if the name is '', this expression returns a
> string instead of a bool, which violates our declared type.

The edge case is impossible, as discussed in review of v2.  I figure the
type checker can't see that, so we need to help it some.  Can we mention
this in the commit message?

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  scripts/qapi/gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index f2e2746fea5..1bad37fc06b 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -243,7 +243,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>  
>      @staticmethod
>      def _is_user_module(name):
> -        return name and not name.startswith('./')
> +        return bool(name and not name.startswith('./'))

           return not (name is None or name.startswith('./')

Looks slightly clearer to me.
>  
>      @staticmethod
>      def _is_builtin_module(name):

On 10/7/20 8:02 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> The edge case is that if the name is '', this expression returns a
>> string instead of a bool, which violates our declared type.
> 
> The edge case is impossible, as discussed in review of v2.  I figure the
> type checker can't see that, so we need to help it some.  Can we mention
> this in the commit message?
> 

Sure. It's quite possible we will be able to better model and constrain 
these types, but that will come after all of these patches.

Mypy just has no way of knowing that '' is forbidden. That constraint 
does not exist in the type system we have here at present.

You could, by the way, create a type called NonEmptyString and create a 
function that casts from str to NonEmptyString by means of an assertion 
and then annotating the return type.

Anyway, I'll update the commit message for now.

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>   scripts/qapi/gen.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index f2e2746fea5..1bad37fc06b 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -243,7 +243,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>>   
>>       @staticmethod
>>       def _is_user_module(name):
>> -        return name and not name.startswith('./')
>> +        return bool(name and not name.startswith('./'))
> 
>             return not (name is None or name.startswith('./')
> 
> Looks slightly clearer to me.
>>   
>>       @staticmethod
>>       def _is_builtin_module(name):