Re: [PATCH v3 3/7] qapi/expr: Split check_expr out from check_exprs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> --0000000000003b01fe05f45a096a
> Content-Type: text/plain; charset="UTF-8"
>
> On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Another observation...
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Primarily, this reduces a nesting level of a particularly long
>> > block. It's mostly code movement, but a new docstring is created.
>> >
>> > It also has the effect of creating a fairly convenient "catch point" in
>> > check_exprs for exception handling without making the nesting level even
>> > worse.
>>
>
> What I mean is: If you want to handle exceptions without this patch, a
> try/catch will add another nesting level to this hundred line function.
>
> By splitting it, you can use the outer looping function as a choke point to
> write the handler.
>
> I had a branch once where I use this fact to stop routing "info" into 99%
> of expr.py. I did this to use an off the shelf JSON validator while
> preserving your custom source info error reporting that identifies the
> precise location of the error.

When you have to reindent anyway, the drawback "makes git-blame less
useful" evaporates.  When the reindent is due to another level of
nesting, the benefit "reduces nesting" becomes more valuable.

> I took that out for now, in the interest of staying focused on the minimum
> viable to achieve strict typing, but found this change to be helpful anyway.
>
> Admit that it does muddy the waters with git blame, but... my calculus on
> that might just be different from yours.
>
> (To me, git blame is almost always something I have to trace back through
> several refactors anyway, so I see the battle as lost before I started.)
>
> I can omit this patch for now if you'd like, it isn't crucial. I just think
> I'd still "kinda like to have it". I'll leave it up to you.

I'd prefer to shelve it for now.  Bring it back when we have to reindent
anyway.

>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> >
>> > ---
>> >
>> > This patch was originally written as part of my effort to factor out
>> > QAPISourceInfo from this file by having expr.py raise a simple
>> > exception, then catch and wrap it at the higher level.
>> >
>> > This series doesn't do that anymore, but reducing the nesting level
>> > still seemed subjectively nice. It's not crucial.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>
> Whoops, script doesn't understand when I add notes.
>
>> ---
>> >  scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
>> >  1 file changed, 95 insertions(+), 84 deletions(-)
>> >
>> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> > index 5a1782b57ea..b56353bdf84 100644
>> > --- a/scripts/qapi/expr.py
>> > +++ b/scripts/qapi/expr.py
>> > @@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info:
>> QAPISourceInfo) -> None:
>> >      check_type(args, info, "'data'", allow_dict=not boxed)
>> >
>> >
>> > +def check_expr(expr_elem: _JSONObject) -> None:
>>
>> [...]
>>
>> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>> >      """
>> >      Validate and normalize a list of parsed QAPI schema expressions.
>>
>> The typing is kind of wrong.
>>
>> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
>> as List[Dict[str, object]].  It is actually a dict mapping
>>
>>    'expr' -> _ExprValue
>>    'info' -> QAPISourceInfo
>>    'doc'  -> Optional[QAPIDoc]
>>
>> Thet's not what _JSONObject is!  Its doc string describes it as
>> "Deserialized JSON objects as returned by the parser", i.e. the same as
>> _ExprValue.
>>
>> Works only because _JSONObject is a dict mapping arbitrary keys to
>> _JSONObject, str or bool.
>>
>> This patch spreads the flawed typing to the new function.
>>
>> Gets healed later in the series.
>>
>
> Oops...!
>
> Symptom of patch reordering that happened some time ago, I think. Mea
> culpa. Will fix.
>
> (Possibly with some ugly intermediate type that goes away by end of series.)

Only if it's dead easy.

Simply have the commit message point out there's problem being fixed?

>> [...]