Re: [PATCH v2 0/7] qapi: static typing conversion, pt5c

[John Snow]

On Wed, Feb 8, 2023 at 11:31 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > This is part five (c), and focuses on sharing strict types between
> > parser.py and expr.py.
> >
> > gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c
> >
> > Every commit should pass with:
> >  - `isort -c qapi/`
> >  - `flake8 qapi/`
> >  - `pylint --rcfile=qapi/pylintrc qapi/`
> >  - `mypy --config-file=qapi/mypy.ini qapi/`
> >
> > Some notes on this series:
> >
> > Patches 2 and 3 are almost entirely superseded by patch 5, but I wasn't
> > as confident that Markus would like patch 5, so these patches aren't
> > squashed quite as tightly as they could be -- I recommend peeking ahead
> > at the cover letters before reviewing the actual patch diffs.
>
> Yes, you're taking a somewhat roundabout path there.

The result of trying 10 different things and seeing what was feasible
through trial and error, and rather less the product of an intentional
design. In the name of just getting the ball rolling again, I sent it
out instead of hemming and hawing over perfection. Publish early,
Publish often! ... is what people doing the publishing say. Apologies
to the reviewer.

>
> I think I like PATCH 5 well enough.  Do you have a tighter squash in
> mind?

Not directly. I could essentially just squash them, but that becomes a
pretty big patch.

>
> > By the end of this series, the only JSON-y types we have left are:
> >
> > (A) QAPIExpression,
> > (B) JSONValue,
> > (C) _ExprValue.
> >
> > The argument I'm making here is that QAPIExpression and JSONValue are
> > distinct enough to warrant having both types (for now, at least); and
> > that _ExprValue is specialized enough to also warrant its inclusion.
> >
> > (Brutal honesty: my attempts at unifying this even further had even more
> > hacks and unsatisfying conclusions, and fully unifying these types
> > should probably wait until we're allowed to rely on some fairly modern
> > Python versions.)
>
> Feels okay to me.

Sorry, best I could do with reasonable effort. I will try to improve
the situation when we bump the Python version!