| Message ID | 20210223003408.964543-16-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt3 | expand |
John Snow <jsnow@redhat.com> writes: > There's not a big obvious difference between the types of checks that > happen in the main function versus the kind that happen in the > functions. Now they're in one place for each of the main types. > > As part of the move, spell out the required and optional keywords so > they're obvious at a glance. Use tuples instead of lists for immutable > data, too. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Cleber Rosa <crosa@redhat.com> No objection to changing read-only lists to tuples (applies to previous patch, too). No objection to turning positional into keyword arguments where that improves clarity. I have doubts on the code motion. Yes, the checks for each type are now together. On the other hand, the check_keys() are now separate. I can no longer see all the keys at a glance.
On 2/25/21 10:28 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> There's not a big obvious difference between the types of checks that >> happen in the main function versus the kind that happen in the >> functions. Now they're in one place for each of the main types. >> >> As part of the move, spell out the required and optional keywords so >> they're obvious at a glance. Use tuples instead of lists for immutable >> data, too. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> Reviewed-by: Cleber Rosa <crosa@redhat.com> > > No objection to changing read-only lists to tuples (applies to previous > patch, too). > > No objection to turning positional into keyword arguments where that > improves clarity. > > I have doubts on the code motion. Yes, the checks for each type are now > together. On the other hand, the check_keys() are now separate. I can > no longer see all the keys at a glance. > I guess it depends on where you wanted to see them; I thought it was strange that in check_foobar I couldn't see what foobar's valid keys were without scrolling back to the bottom of the file. Needing to see all the keys for the disparate forms together was not a case I ran into, but you can always drop this patch for now if you'd like. I had some more adventurous patches that keeps pushing in this direction, but I don't know if it's really important. My appetite in this area has waned since November. --js
John Snow <jsnow@redhat.com> writes: > On 2/25/21 10:28 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> There's not a big obvious difference between the types of checks that >>> happen in the main function versus the kind that happen in the >>> functions. Now they're in one place for each of the main types. >>> >>> As part of the move, spell out the required and optional keywords so >>> they're obvious at a glance. Use tuples instead of lists for immutable >>> data, too. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> Reviewed-by: Cleber Rosa <crosa@redhat.com> >> >> No objection to changing read-only lists to tuples (applies to previous >> patch, too). >> >> No objection to turning positional into keyword arguments where that >> improves clarity. >> >> I have doubts on the code motion. Yes, the checks for each type are now >> together. On the other hand, the check_keys() are now separate. I can >> no longer see all the keys at a glance. >> > > I guess it depends on where you wanted to see them; I thought it was > strange that in check_foobar I couldn't see what foobar's valid keys > were without scrolling back to the bottom of the file. > > Needing to see all the keys for the disparate forms together was not a > case I ran into, but you can always drop this patch for now if you'd > like. Let's shelve it for now. > I had some more adventurous patches that keeps pushing in this > direction, but I don't know if it's really important. When I work on a something, I tend to accumulate semi-related cleanups. Including them is rarely a problem for reviewers when the result is two dozen patches or so. When this isn't the case, I can: * Pick them into a separate cleanup series to go before the real work. Risks delaying the real work. * Funnel them onto a cleanup branch to flushed later. Risks lonely death in a rotting branch. * Force myself to abstain from improving things that could really use improvement. I call this "sitting on my hands". This patch is in part three of at least six. Almost 90 patches up to part three, with many more to come. I'm *desperate* to limit scope to not get overwhelmed. Please consider the remedies above. This is a cry for help, not a demand. > My appetite in > this area has waned since November. I understand.
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 61699de8cd5..3672637487b 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -344,6 +344,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None: :param expr: `Expression` to validate. :param info: QAPI source file information. """ + check_keys(expr, info, 'enum', + required=('enum', 'data'), + optional=('if', 'features', 'prefix')) + name = expr['enum'] members = expr['data'] prefix = expr.get('prefix') @@ -374,6 +378,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None: :param expr: `Expression` to validate. :param info: QAPI source file information. """ + check_keys(expr, info, 'struct', + required=('struct', 'data'), + optional=('base', 'if', 'features')) + normalize_members(expr['data']) + name = cast(str, expr['struct']) # Asserted in check_exprs members = expr['data'] @@ -388,6 +397,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None: :param expr: `Expression` to validate. :param info: QAPI source file information. """ + check_keys(expr, info, 'union', + required=('union', 'data'), + optional=('base', 'discriminator', 'if', 'features')) + + normalize_members(expr.get('base')) + normalize_members(expr['data']) + name = cast(str, expr['union']) # Asserted in check_exprs base = expr.get('base') discriminator = expr.get('discriminator') @@ -420,6 +436,11 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None: :param expr: Expression to validate. :param info: QAPI source file information. """ + check_keys(expr, info, 'alternate', + required=('alternate', 'data'), + optional=('if', 'features')) + normalize_members(expr['data']) + members = expr['data'] if not members: @@ -443,6 +464,13 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None: :param expr: `Expression` to validate. :param info: QAPI source file information. """ + check_keys(expr, info, 'command', + required=('command',), + optional=('data', 'returns', 'boxed', 'if', 'features', + 'gen', 'success-response', 'allow-oob', + 'allow-preconfig', 'coroutine')) + normalize_members(expr.get('data')) + args = expr.get('data') rets = expr.get('returns') boxed = expr.get('boxed', False) @@ -464,6 +492,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None: :if: ``Optional[Ifcond]`` (see: `check_if`) :features: ``Optional[Features]`` (see: `check_features`) """ + check_keys(expr, info, 'event', + required=('event',), + optional=('data', 'boxed', 'if', 'features')) + normalize_members(expr.get('data')) + args = expr.get('data') boxed = expr.get('boxed', False) @@ -531,38 +564,16 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: "documentation comment required") if meta == 'enum': - check_keys(expr, info, meta, - ['enum', 'data'], ['if', 'features', 'prefix']) check_enum(expr, info) elif meta == 'union': - check_keys(expr, info, meta, - ['union', 'data'], - ['base', 'discriminator', 'if', 'features']) - normalize_members(expr.get('base')) - normalize_members(expr['data']) check_union(expr, info) elif meta == 'alternate': - check_keys(expr, info, meta, - ['alternate', 'data'], ['if', 'features']) - normalize_members(expr['data']) check_alternate(expr, info) elif meta == 'struct': - check_keys(expr, info, meta, - ['struct', 'data'], ['base', 'if', 'features']) - normalize_members(expr['data']) check_struct(expr, info) elif meta == 'command': - check_keys(expr, info, meta, - ['command'], - ['data', 'returns', 'boxed', 'if', 'features', - 'gen', 'success-response', 'allow-oob', - 'allow-preconfig', 'coroutine']) - normalize_members(expr.get('data')) check_command(expr, info) elif meta == 'event': - check_keys(expr, info, meta, - ['event'], ['data', 'boxed', 'if', 'features']) - normalize_members(expr.get('data')) check_event(expr, info) else: assert False, 'unexpected meta type'