John Snow <js...@redhat.com> writes: > On 1/20/21 9:20 AM, Markus Armbruster wrote: >> John Snow <js...@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: