John Snow <js...@redhat.com> writes:
> On 4/24/21 4:33 AM, Markus Armbruster wrote:
>> The second operand of assert provides no additional information. Please
>> drop it.
>
> I don't agree with "no additional information", strictly.
>
> I left you a comment on gitlab before you started reviewing on-list.
> What I wrote there:
>
> "Markus: I know you're not a fan of these, but I wanted a suggestion on
> how to explain why this must be true in case it wasn't obvious to
> someone else in the future."
But the second operand doesn't explain anything. Look:
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f519518075e..c75434e75a5 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -303,6 +303,7 @@ def get_doc(self, info):
cur_doc = QAPIDoc(self, info)
self.accept(False)
while self.tok == '#':
+ assert isinstance(self.val, str), "Expected str value"
if self.val.startswith('##'):
# End of doc comment
if self.val != '##':
The second operand paraphrases the first one in prose rather than code.
An actual *explanation* would instead tell me why the first operand must
be true.
To do that, I'd point to self.accept()'s postcondition. Which
(informally) is
self.tok in ('#', '{', ... )
and self.tok == '#' implies self.val is a str
and self.tok == '{' implies self.val is None
...
I believe this is required working knowledge for understanding the
parser. Your PATCH 16 puts it in a doc string, so readers don't have to
extract it from code. Makes sense.
It's not going to fit into a workable second operand here, I'm afraid.
I assume you need this assertion for mypy. If yes, let's get the job
done with minimal fuss. If no, please drop the assertion entirely.
