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 <js...@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. -- Eduardo