Code review comment for lp:~pitti/autopilot/print-tree

> 1)
>
> 16 + def print_tree(self, output=None, maxdepth=None, _curdepth=0):
>
> Instead of making the default value for 'output' be 'None', why not make it
> 'sys.stdout' ? That way, you can delete this:
>
> 39 + if output is None:
> 40 + output = sys.stdout

Python has a rather unintuitive semantics when trying to provide non-trivial values as default argument values (First time I ran into that I actually understood it, and just avoid it ever since). In this case as well, if you actually do the change you'll notice that the tests will fail and you'll get the test tree literally dumped to stdout during the tests (although it's supposed to go into a StringIO).

> And the docs should say that output should be a "file-like object that
> supports the 'write' method. That way you can delete these lines as well:
>
> 41 + elif isinstance(output, six.string_types):
> 42 + output = open(output, 'w')

This is a debugging-only function, so it ought to be as convenient as possible. Thus I would also like to support

  mywidget.print_tree("/tmp/dump.txt")

instead of requiring the user to write something like

  with open("/tmp/dump.txt", "w") as f:
     mywidget.print_tree(f)

> I'm not sure that using four spaces as an indentation is a good idea - trees
> can get pretty large... have you tried this one a bug tree like Unity 7?

I didn't, but I suppose it would take half an hour to do that :-) It already takes several seconds for relatively small trees.

> Perhaps 2 spaces would be better? I'm really not sure either way - but I 'd
> like to see some larger output....

Yes, good point. I reduced it.