[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module |
Date: |
Thu, 21 Jan 2021 08:40:14 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
John Snow <jsnow@redhat.com> writes:
> On 1/20/21 9:20 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index 55acd7e080d..b5505685e6e 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
[...]
>>> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) ->
>>> None:
>>> self._genc = None
>>> self._genh = None
>>> else:
>>> - self._add_user_module(module.name, self._user_blurb)
>>> + assert module.user_module, "Unexpected system module"
>> The string provides no value.
>>
>
> That's just, like, your opinion, man. It has value to me.
>
>
> Compare:
>
> ```
> #!/usr/bin/env python3
>
> def main():
> assert False
>
> if __name__ == '__main__':
> main()
> ```
>
> ```
> # ./foo.py
>
> Traceback (most recent call last):
> File "./foo.py", line 7, in <module>
> main()
> File "./foo.py", line 4, in main
> assert False
> AssertionError
> ```
>
> With:
>
>
> ```
> #!/usr/bin/env python3
>
> def main():
> assert False, "Unexpected system module"
>
> if __name__ == '__main__':
> main()
> ```
>
> ```
> # ./foo.py
>
> Traceback (most recent call last):
> File "./foo.py", line 7, in <module>
> main()
> File "./foo.py", line 4, in main
> assert False, "Unexpected system module"
> AssertionError: Unexpected system module
> ```
>
> I like the extra string for giving some semantic context as to
> specifically what broke (We don't expect to see system modules here)
> instead of just a stack trace.
Your test uses assert with an argument that tells us nothing. But the
assert we're arguing about has a nice, expressive argument.
The string improves the report from
File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module
assert module.user_module
AssertionError
to
File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module
assert module.user_module, "Unexpected system module"
AssertionError: Unexpected system module
Even if you value the difference, I very much doubt it justifies the
clutter. Also, slippery slope towards pigs wearing way too much
lipstick.
Tested with
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index f3f4d2b011..bbfb30dc5a 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -321,6 +321,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
# be generated.
self._current_module = None
else:
+ module.name = "./HACK"
assert module.user_module, "Unexpected system module"
self._add_module(module.name, self._user_blurb)
self._begin_user_module(module.name)
>
>>> + self._add_module(module.name, self._user_blurb)
>>> self._begin_user_module(module.name)
>>> def visit_include(self, name: str, info: QAPISourceInfo) ->
>>> None:
- Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str, (continued)
[PATCH v3 07/17] qapi/gen: Replace ._begin_system_module(), John Snow, 2021/01/19
[PATCH v3 11/17] qapi: centralize the built-in module name definition, John Snow, 2021/01/19
[PATCH v3 04/17] qapi/gen: inline _wrap_ifcond into end_if(), John Snow, 2021/01/19
[PATCH v3 12/17] qapi/gen: write _genc/_genh access shims, John Snow, 2021/01/19
[PATCH v3 16/17] qapi: type 'info' as Optional[QAPISourceInfo], John Snow, 2021/01/19
[PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module, John Snow, 2021/01/19
[PATCH v3 02/17] qapi/events: fix visit_event typing, John Snow, 2021/01/19
[PATCH v3 09/17] qapi: use './builtin' as the built-in module name, John Snow, 2021/01/19
[PATCH v3 15/17] qapi/gen: Drop support for QAPIGen without a file name, John Snow, 2021/01/19
[PATCH v3 13/17] qapi/gen: Support for switching to another module temporarily, John Snow, 2021/01/19
[PATCH v3 14/17] qapi/commands: Simplify command registry generation, John Snow, 2021/01/19
[PATCH v3 17/17] qapi: enable strict-optional checks, John Snow, 2021/01/19