| Message ID | 20201005195158.2348217-27-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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):
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):