Re: [PATCH 05/22] qapi/parser: Assert lexer value is a string

[Markus Armbruster]

John Snow <jsnow@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.