On Wed, Sep 30, 2020 at 02:32:49PM -0400, John Snow wrote:
On 9/30/20 2:24 PM, Eduardo Habkost wrote:
On Wed, Sep 30, 2020 at 12:31:43AM -0400, John Snow wrote:
Returning a *something* or a Tuple of *something* is hard to accurately
type. Let's just always return a tuple for structural consistency.
Instances of the 'TreeNode' type can be replaced with the slightly more
specific 'AnnotatedNode' type.
Signed-off-by: John Snow <jsnow@redhat.com>
So, the only place where this seems to make a difference is
_tree_to_qlit().
We just need to prove that
_tree_to_qlit(o, ...)
will have exactly the same result as
_tree_to_qlit((o, None), ...).
For reference, this is the beginning of _tree_to_qlit():
| def _tree_to_qlit(obj: TreeNode,
| level: int = 0,
| suppress_first_indent: bool = False) -> str:
|
| def indent(level: int) -> str:
| return level * 4 * ' '
|
| if isinstance(obj, tuple):
| ifobj, extra = obj
`obj` is the return value of _make_tree()
`ifobj` is the original `obj` argument to _make_tree().
| ifcond = extra.get('if')
ifcond will be None.
| comment = extra.get('comment')
comment will be None
| ret = ''
| if comment:
| ret += indent(level) + '/* %s */\n' % comment
nop
| if ifcond:
| ret += gen_if(ifcond)
nop
| ret += _tree_to_qlit(ifobj, level)
ret will be '', so this is equivalent to:
ret = _tree_to_qlit(ifobj, level)
which is almost good.
The only difference seems to that suppress_first_indent=True will
be ignored. We should pass suppress_first_indent as argument in
the recursive call above, just in case.
This is a really good spot, and I indeed hadn't considered it at all when I
did this.
(I simply made the change and observed it worked just fine!)
The existing code will behave weirdly if there are comments or
conditions and suppress_first_indent=True, but I suggest we try
to address this issue later.
| if ifcond:
| ret += '\n' + gen_endif(ifcond)
nop
| return ret
Hm, yes, it's a hypothetical case, but perhaps we can use an assertion to
help guard against it if development creates that case later by accident.
That ought to be good enough for now to not waste time accommodating a
(presently) fictional circumstance?
Thanks for the good sleuthing here.
With the current code, both
ret += _tree_to_qlit(ifobj, level)
and
ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
will behave exactly the same.
The former will behave weirdly if we wrap a dictionary value using
_tree_node(). We don't do that today.
The latter will behave weirdly if there's a comment or ifcond
attached in a dictionary value. We don't do that today.
I believe the latter is less likely to be triggered by accident.
But I'd be happy with either:
# _make_tree() shouldn't be use to wrap nodes that
# may be printed using suppress_first_indent=True
# (in other words, dictionary values shouldn't be wrapped using _make_tree())
assert(not suppress_first_indent)
ret += _tree_to_qlit(ifobj, level)
or
# we can't add ifcond or comments to nodes that may be
# printed using suppress_first_indent=True
# (in other words, dictionary values can't have ifcond or comments)
assert(not suppress_first_indent or (not comment and not ifcond))
ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
If we have time to spare later, we could do this:
def _value_to_qlit(obj: Union[None, str, Dict[str, object], List[object],
bool],
level: int = 0,
suppress_first_indent: bool = False) -> str:
...
if obj is None:
...
elif isinstance(obj, str):
...
elif isinstance(obj, list):
...
...
def _tree_to_qlit(obj: TreeNode, level: int = 0) -> str:
if isinstance(obj, AnnotatedNode):
...
else:
return _value_to_qlit(obj, level)
This way, it will be impossible to set suppress_first_indent=True
on an annotated node.