Hi, this patchset enables strict optional checking in mypy for
everything we have typed so far.
In general, this patchset seeks to clarify Optional[T] vs T and lift the
no-strict-optional restriction in mypy.
Ironing out these issues allows us to be even stricter with our type
checking, which improves consistency in subclass interface types and
should make review a little nicer.
This series is based on (but does not require) the 'qapi: sphinx-autodoc
for qapi module' series.
V4:
001/16:[----] [--] 'qapi/commands: assert arg_type is not None'
002/16:[----] [--] 'qapi/events: fix visit_event typing'
003/16:[----] [--] 'qapi/main: handle theoretical None-return from re.match()'
004/16:[----] [--] 'qapi/gen: inline _wrap_ifcond into end_if()'
005/16:[0047] [FC] 'qapi: centralize is_[user|system|builtin]_module methods'
006/16:[0007] [FC] 'qapi/gen: Replace ._begin_system_module()'
007/16:[----] [--] 'qapi: use explicitly internal module names'
008/16:[0016] [FC] 'qapi: use './builtin' as the built-in module name'
009/16:[0011] [FC] 'qapi/gen: Combine ._add_[user|system]_module'
010/16:[0008] [FC] 'qapi: centralize the built-in module name definition'
011/16:[----] [-C] 'qapi/gen: write _genc/_genh access shims'
012/16:[----] [--] 'qapi/gen: Support for switching to another module temporarily'
013/16:[----] [--] 'qapi/commands: Simplify command registry generation'
014/16:[----] [--] 'qapi/gen: Drop support for QAPIGen without a file name'
015/16:[----] [-C] 'qapi: type 'info' as Optional[QAPISourceInfo]'
016/16:[----] [--] 'qapi: enable strict-optional checks'
Removed the patch that made visit_module work in terms of QAPISchemaModule
instead of the module name.
05: Remove the properties, keep just the static/classmethods.
Convert classmethod to staticmethod where possible.
Add docstrings to alleviate confusion about what these methods do.
Leave qapidoc generator alone for now.
Remove future-bleed from is_system_module (check for None, not "./builtin")
06: Change the conditionals in gen.py visit_module to use is_builtin_module.
08: Some contextual difference now that modules are no longer being passed
via visit_module.
Code added by patch 05 is now amended here to make it consistent in history.
09: Removed assertion. Changed commit message.
10: Contextual fallout; removed end-of-line comment that was unneeded.
Notes:
Admittedly, at this point it feels like style and taste. I played with
several other alternatives, but felt I wasn't making good progress in
any direction that was more defensible than what I have here.
If there are nits at this point, I think they generally require more
engineering than what is present here to resolve them satisfactorily; to
that end I feel like this is close to the fewest SLOC changes that felt
defensible. Concrete counter-proposals would be preferred to minimize
guess-work when it comes to matters of style.
Namely, I would vastly prefer "module.system_module" as a property of
the module object and to pass those to visitors, but the work involved
in doing this is not trivial, because the generators that "add a system
module" generally only add a system module *name* to the generator, as
if it was visited -- but this is an emulated behavior as a
code-generation technique, the module does not exist. Changing the
gen.py functions to *actually* create a module is more hassle than it is
worth at present, so that python module ought to remain working in terms
of module *names*. Renaming the functions to reflect that made them a
little too cumbersome/long for my tastes, so I left them alone.
However, I still felt it important to begin making the claim that module
namespaces are global and shared between the generator and the schema;
so I still opted to use QAPISchemaModule methods to formalize the module
type definitions, even if we aren't using them in a classy way (yet).
You may argue that these can be functions instead, and they can; but it
is just an organizational preference that they live within the
QAPISchemaModule namespace because (to me) this solidifies the idea that
schema.py and QAPISchemaModule themselves are the solely responsible
entities for these definitions. (i.e., it makes it clear to me that
these namespaces aren't purely an invention of gen.py -- which they can
no longer be, as we are declaring and naming "./builtin", the built-in
schema module that contains our predefined types in schema.py.)
I didn't think it was worth making a class for the names alone and going
that route; that felt like more work than was warranted, too. So this
represents roughly the bottom floor of what seemed subjectively
sensible.
--js
John Snow (11):
qapi/commands: assert arg_type is not None
qapi/events: fix visit_event typing
qapi/main: handle theoretical None-return from re.match()
qapi/gen: inline _wrap_ifcond into end_if()
qapi: centralize is_[user|system|builtin]_module methods
qapi: use explicitly internal module names
qapi: use './builtin' as the built-in module name
qapi: centralize the built-in module name definition
qapi/gen: write _genc/_genh access shims
qapi: type 'info' as Optional[QAPISourceInfo]
qapi: enable strict-optional checks
Markus Armbruster (5):
qapi/gen: Replace ._begin_system_module()
qapi/gen: Combine ._add_[user|system]_module
qapi/gen: Support for switching to another module temporarily
qapi/commands: Simplify command registry generation
qapi/gen: Drop support for QAPIGen without a file name
scripts/qapi/commands.py | 62 ++++++++--------
scripts/qapi/events.py | 16 ++--
scripts/qapi/gen.py | 94 ++++++++++++------------
scripts/qapi/main.py | 2 +
scripts/qapi/mypy.ini | 1 -
scripts/qapi/schema.py | 42 +++++++++--
scripts/qapi/types.py | 4 +-
scripts/qapi/visit.py | 6 +-
tests/qapi-schema/comments.out | 2 +-
tests/qapi-schema/doc-good.out | 2 +-
tests/qapi-schema/empty.out | 2 +-
tests/qapi-schema/event-case.out | 2 +-
tests/qapi-schema/include-repetition.out | 2 +-
tests/qapi-schema/include-simple.out | 2 +-
tests/qapi-schema/indented-expr.out | 2 +-
tests/qapi-schema/qapi-schema-test.out | 2 +-
16 files changed, 139 insertions(+), 104 deletions(-)
--
2.29.2