Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 1/20/21 7:07 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>>> index e03abcbb959..f754f675d66 100644
>>> --- a/docs/sphinx/qapidoc.py
>>> +++ b/docs/sphinx/qapidoc.py
>>> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
>>>           self._env = env
>>>           self._qapidir = qapidir
>>>   -    def visit_module(self, name):
>>> -        if name is not None:
>>> -            qapifile = self._qapidir + '/' + name
>>> +    def visit_module(self, module):
>>> +        if module.name:
>> Replacing the "is not None" test by (implicit) "is thruthy" changes
>> behavior for the empty string.  Intentional?
>> 
>
> Instinctively it was intentional, consciously it wasn't. I was worried
> about what "qapifile" would produce if the string happened to be
> empty.

It would produce a dependency on the directory.

>> I've had the "pleasure" of debugging empty strings getting interpreted
>> like None where they should be interpreted like any other string.
>> 
>
> assert module.name, then?

Let's avoid changing behavior in a refactoring patch.  If you want an
assertion to ease your worry, separate patch.

>>> +            qapifile = self._qapidir + '/' + module.name
>>>               self._env.note_dependency(os.path.abspath(qapifile))
>>> -        super().visit_module(name)
>>> +        super().visit_module(module)
>>>     
>> [...]
>>