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.
OK. Let me peek ahead to some of my other pending patches and just confirm there's no stronger reason to keep it...
(/me still ~kiiiinda~ wants it, but admits its not seemingly crucial atm)
>> > 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?
>> [...]